-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set up CI: test, analyze, and generate #60
Comments
This happens automatically if you rerun build_runner. Seems like its omission was basically a mismerge between 901a99e, when it was merged early from zulip#84: zulip#84 (comment) and 780b092 / zulip#22, which added this file. Chalk it up as another occasion where zulip#60 would have helped keep things clean.
This happens automatically if you rerun build_runner. Seems like its omission was basically a mismerge between 901a99e, when it was merged early from zulip#84: zulip#84 (comment) and 780b092 / zulip#22, which added this file. Chalk it up as another occasion where zulip#60 would have helped keep things clean.
Another form of generated file we'll want to include:
|
This provides the guts of an implementation of zulip#60. 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: dart-lang/sdk#44573 dart-lang/sdk#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. --- Fortunately, using Bash does mean we get to reuse the work that's already gone into the `tools/test` script in zulip-mobile. This commit introduces a bare-bones version, with most of the features (notably --diff and its friends) stripped out, and the test suites replaced with a first two for our codebase. This version also uses `set -u` and `set -o pipefail`, unlike the one in zulip-mobile, in addition to `set -e`.
These two were included in #314. (I called the script 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
|
This one I left out in #314, for reasons described at 477fe67 :
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 |
We should run our tests and other checks in a CI setup.
These consist of:
flutter test
flutter analyze
build_runner
command mentioned in the README (but can usebuild
instead ofwatch
).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):
tools/test
in zulip-mobile, that makes all the same checks we do in CI.tools/test
in zulip-mobile) efficient about not redoing checks for things that shouldn't be affected by the local changes.The text was updated successfully, but these errors were encountered: