Skip to content
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

Closed
gnprice opened this issue Apr 10, 2023 · 3 comments · Fixed by #314
Closed

Set up CI: test, analyze, and generate #60

gnprice opened this issue Apr 10, 2023 · 3 comments · Fixed by #314
Assignees
Labels
a-tools Our own development tooling, scripts, and infrastructure
Milestone

Comments

@gnprice
Copy link
Member

gnprice commented Apr 10, 2023

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 Store some user data locally #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.
@gnprice gnprice added this to the Alpha milestone May 27, 2023
@gnprice gnprice added a-tools Our own development tooling, scripts, and infrastructure and removed m-alpha labels May 27, 2023
gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 30, 2023
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.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 30, 2023
The upgrade to drift_dev 2.8.x seems to be required in order to
work with the upgrade to analyzer 5.12.0 that we took in 861a929.
The others are in turn required for that.

This makes another occasion where having CI, zulip#60, would have helped
keep things clean for us.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 31, 2023
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.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 31, 2023
The upgrade to drift_dev 2.8.x seems to be required in order to
work with the upgrade to analyzer 5.12.0 that we took in 861a929.
The others are in turn required for that.

This makes another occasion where having CI, zulip#60, would have helped
keep things clean for us.
@gnprice
Copy link
Member Author

gnprice commented Jul 14, 2023

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

@gnprice gnprice self-assigned this Sep 12, 2023
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Oct 3, 2023
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`.
@gnprice gnprice closed this as completed in 47e1993 Oct 6, 2023
@gnprice
Copy link
Member Author

gnprice commented Oct 6, 2023

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.

@gnprice
Copy link
Member Author

gnprice commented Oct 6, 2023

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-tools Our own development tooling, scripts, and infrastructure
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant