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 usebuild
instead ofwatch
). - 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.
- The
(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 runflutter 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 runtools/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.