zulip/zulip-flutter

Set up CI: test, analyze, and generate

gnprice opened this issue · 3 comments

We should run our tests and other checks in a CI setup.

These consist of:

  • flutter test
  • flutter analyze
  • Regenerating all generated files, and checking that nothing changed. This means:
    • The build_runner command mentioned in the README (but can use build instead of watch).
    • After #13, a couple of drift_dev commands that need to be run whenever the database schema changes; they'll be described in a comment at the spot where we define the current schema version.

(If we complete this before #13, that's fine; we'll leave out that last item, and just add it as part of #13.)

Some things that we'd ideally have but are optional for this issue (and we'll just file followup issues if they're not in the first implementation of this one):

  • A handy wrapper script for local development use, like tools/test in zulip-mobile, that makes all the same checks we do in CI.
  • That wrapper script being (again like tools/test in zulip-mobile) efficient about not redoing checks for things that shouldn't be affected by the local changes.
  • That wrapper script being written in Dart, rather than Bash.

Another form of generated file we'll want to include:

  • the icon font and corresponding IconData code, from #200

Some things that we'd ideally have but are optional for this issue (and we'll just file followup issues if they're not in the first implementation of this one):

  • A handy wrapper script for local development use, like tools/test in zulip-mobile, that makes all the same checks we do in CI.
  • That wrapper script being (again like tools/test in zulip-mobile) efficient about not redoing checks for things that shouldn't be affected by the local changes.

These two were included in #314. (I called the script tools/check, to avoid reusing the word "test" when flutter test is one of the several suites the script runs.)

It's not as efficient about not redoing checks as the one in zulip-mobile is. It's pretty good about all the miscellaneous suites where the whole suite only relates to a small part of our codebase; but for the two major suites flutter analyze and flutter test, the bulk of our code can affect the suite, and so to get this kind of optimization one needs support within those respective tools for only redoing the portion of the work that could be affected by the changed files. And unlike Jest, Flow, ESLint, or Prettier, the Dart analyzer and Dart test runner don't have that feature as far as I've found. So:

  • For analyzer results, a recommended workflow is to watch the analyzer results in your IDE; these come from a server that stays in memory and responds quickly to incremental edits. (Essentially the same architecture as Flow uses, except that Flow has the further feature that its CLI talks to the same server and so can often give instant results.)
  • For test results, flutter test does cheerfully accept a list of test files to run — it just doesn't have Jest's fancy support for computing that list automatically as the transitive importers of a set of changed source files. So probably what one usually wants in development is still to run flutter test test/foo/bar_test.dart for a specific test file or set of test files that you expect your changes to affect. Still should be handy to run tools/check as part of a less-tight iteration loop, though — like in particular before submitting a PR, or before merging a rebased PR.
  • That wrapper script being written in Dart, rather than Bash.

This one I left out in #314, for reasons described at 477fe67 :

I'd have liked to write this in Dart, rather than in shell.
But when running this script interactively (vs. in CI),
a key ingredient for a good developer experience is that the
child processes like `flutter test` should have their stdout
and stderr connected directly to those of the parent script,
i.e. to the developer's terminal.  Crucially, that lets those
processes know to print their output in a form that takes advantage
of terminal features to get a good interactive experience.
The gold standard for a CLI tool is to have a way to control
that choice directly, but `flutter test` and some other
Dart-based tools lack that capability.

And in Dart, as far as I can tell, there simply is no way to
start a child process that inherits any of the parent's file
handles; instead the child's output will always be wired up to
pipes for the parent to consume.  There's advice for how the
parent can copy the output, chunk by chunk, to its own output:
  https://github.com/dart-lang/sdk/issues/44573
  https://github.com/dart-lang/sdk/issues/5607
  https://stackoverflow.com/questions/72183086/dart-dont-buffer-process-stdout-tell-process-that-it-is-running-from-a-termina

but that doesn't solve the problem of the child's behavior
changing when it sees a pipe instead of a terminal.

The nastiest consequence of this behavior is that the output of
`flutter test` is hundreds of lines long, even for our small codebase,
even when only a single test case fails or none at all. That's
fine in CI, but pretty much intolerable for a development workflow.

So, shell it is.  (Python, or Javascript with Node, or many other
languages would also handle this just fine, but would mean an extra
system of dependencies to manage.)  Specifically, Bash.

So unfortunately Dart is not a suitable language to write this script in, due both to deficiencies in Dart for use in a CLI and deficiencies in the CLIs of Dart development tools like dart test (the basis of flutter test). And those don't seem likely to be fixed anytime soon. So we'll just stick with Bash unless that changes.