Add a run-all-tests script, for local and CI use #314
Conversation
This change happens when rerunning the `dart run drift_dev` commands prescribed in the comments at [AppDatabase.schemaVersion]. The discrepancy appears to have been there since this schema file was introduced in commit 066fe25, oops. After pretty-printing (with `jq`), the diff looks like this: @@ -38,3 +38,7 @@ "name": "realm_url", "default_client_dart": null, - "dsl_features": [] + "dsl_features": [], + "type_converter": { + "dart_expr": "const UriConverter()", + "dart_type_name": "Uri" + } }, So it reflects the use of UriConverter here: Column<String> get realmUrl => text().map(const UriConverter())(); Probably what happened is that I generated the schema file while on a draft of that branch that didn't yet use UriConverter there; then added it and didn't realize it would affect the schema file. We'll shortly have an omnibus run-all-checks script which in particular reruns those files and checks that nothing changed; and we'll then run that script in CI, which will prevent any discrepancies like this one from creeping in in the future. A draft of that script is how I discovered this.
Either name "scripts" or "tools" works well as a description for what this directory contains: scripts we run as development tools. When I first created it with this build-icon-font script, I chose "scripts" for reasons of tab-completion. Specifically, when completing a filename to pass to `git` or `flutter test` or etc., I wanted to preserve for the `test/` directory the privilege of being completable after a single keystroke: "t<TAB>" completes to "test/". After all, that's a directory one wants to talk about all the time. The name "scripts", though, turns out to be quite unfortunate for a different case of tab-completion: when completing a command name. If you want to run `scripts/icons/build-icon-font`, you pretty much have to type out "scripts/" in full. It's competing not only with other files in the tree, but with commands in your PATH; and my PATH has commands "script" and "scriptreplay", as well as commands whose names diverge from "scripts" after "s, "sc", or "scr". That's fine when it's just about `scripts/icons/build-icon-font`, because that's a command one runs only very infrequently. But we're about to add an omnibus run-all-checks script; that'll be used in CI, but I also want it to be convenient to run frequently in local development when iterating on changes, just as we do in zulip-mobile with `tools/test`. To live up to that, I want its name to be as quick and easy to type as possible. And so, `tools/`. Now completing "test/" for a filename will take three keystrokes "te<TAB>" instead of two; but in exchange, this tool-scripts directory will take only four keystrokes "too<TAB>" to get "tools/", even when completing as a command name.
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 are borrowed from zulip-mobile's tools/tsflower script, and slightly extended. The latter two functions don't have names there, but correspond to logic I worked out and wrote down there.
The zulip-mobile version adds a tiny CLI around these; but I think we can skip that and have this just be a collection of shell functions to be sourced by whatever script needs them.
Other suites still run all the time, effectively using the `--all` behavior unconditionally.
This is based on a similar suite I drafted for zulip-mobile back in... 2019, yikes. It looks like at the time the thing that stopped me from merging it there was the question of how to handle Shellcheck as a dependency. But that needn't be a barrier to merging it -- it just needs to go into the list of "extra suites" that don't run by default. As a bonus we can give a nice error message when missing.
|
Hmm, GitHub doesn't seem to be picking up the workflow here to run it. Puzzling because it works fine on my fork gnprice/zulip-flutter: I'll poke around the GitHub settings some more. But in any event, the code should all be ready for review. |
I believe it's caused by this being a public repo: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories |
|
Chat thread for debugging: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/GitHub.20settings.20for.20CI/near/1653618 |
|
Well, it's run now 🤷, and passed. What I did was to push the PR branch to a temporary ad hoc branch within this repo ( I think I've done enough investigation of GitHub's behavior for now. Given that it works when pushing to a branch here, this should definitely be enough that we get CI post-merge, i.e. upon pushing to |
|
Here's output I get from What's Also I wonder about addressing that warning about "Your current |
|
Otherwise LGTM! We can always fine-tune things later. One thing I can envision wanting in my routine workflow is that it doesn't waste time setting up and running our unit tests if some bug makes that a pointless exercise and that bug was already flagged by the analyzer. Probably a fine approximation would be to just not run the |
|
Thanks for the review!
Hmm interesting. Yeah, we don't have anything for it to do there — but I suppose it needs to scan those files to find out that that's the case. I'd expect that that scanning is fast, and that whatever took it 15+ seconds there was some kind of startup work that would have been the same regardless of which files were in the queue. In particular, that warning about the
Yeah, definitely. I'd noticed that occasionally before, but it is a bit more annoying now in the context of this
That's a bit tricky because the This is the reason why it runs |
Makes sense; ISTR doing that frequently when working in zulip-mobile. |
Fixes #60.
This introduces a script
tools/checkto run all our forms of tests.Much like the
tools/testin zulip-mobile, its CLI is optimized by default for local development; so you can type justtools/checkat your command line, and it'll do its best to run just the relevant tests and emit helpful, low-noise output. Seetools/check --helpfor further options.And then this also adds a GitHub Actions workflow to run our tests for CI. After setup, the core of this is
tools/check --all --verbose.There's in fact one spot where we had a generated file that was out of sync with its source! The new test script catches that, and it'll help us avoid any similar discrepancy in the future. The first commit of this branch fixes the existing issue.