ci(dart): analyze, format and test the packages under dart/ - #2494
ci(dart): analyze, format and test the packages under dart/#2494diegolopezrm wants to merge 4 commits into
Conversation
`unnecessary_library_directive` flags all three: a `library;` with no doc comment or annotation carries no meaning. The two libraries in a2ui_core get the doc comment they were missing — which also gives pub.dev something to show — and a2ui_agent's, which re-exports a single file, loses the directive. This clears the last analyzer infos in both packages, so the CI job added next can run `dart analyze --fatal-infos`.
The Flutter workflow builds its matrix from `find samples -name pubspec.yaml`, so `dart/a2ui_core` and `dart/a2ui_agent` never enter it, and no other workflow runs `dart analyze` or `dart test`. The formatting step in fix_format.sh covers `samples/client/flutter` and `renderers/flutter`, not `dart/`. The two packages that ship to pub.dev were the only source tree in the repository with no CI at all. This adds a workflow that holds them to the same bar as the samples: formatting, `dart analyze --fatal-infos`, and the package's own tests, one matrix entry per package so a new Dart package is picked up automatically. It installs Flutter through the repository's setup-dart action rather than a Dart-only SDK, because the packages resolve through the monorepo workspace whose root pubspec declares a Flutter constraint (a2ui-project#2463).
There was a problem hiding this comment.
Code Review
This pull request cleans up the Dart library declarations by removing an unnecessary empty library directive in a2ui_agent.dart and adding descriptive library-level documentation comments to a2ui_core.dart and reactivity.dart. There are no review comments, so I have no feedback to provide.
# Conflicts: # dart/a2ui_core/lib/a2ui_core.dart
`dart format --set-exit-if-changed` over dart/ fails on validator_test.dart and message_processor_conformance_test.dart, which arrived with a2ui-project#2439. fix_format.sh formats samples/client/flutter and renderers/flutter, never dart/, so nothing has been holding these packages to a format until the job this PR adds. Pure `dart format` output, with the same result under --language-version=3.10, so it is not a language-version difference.
|
Rebased onto main after #2439. Two changes to what this PR contains. The library-directive commit is gone. #2439 resolved all three the same way, so It picked up a two-file formatting fix, which is worth a sentence because it is the first thing the new job found. Both arrived with #2439, and neither is anyone's oversight: That is the argument for the PR in miniature: the packages published from this repository have had no formatting, analysis or test gate, and the gap shows up the moment one is applied. Verified after the rebase: |
|
Hi @gspencergoog, I have pulled and verified the changes locally, and all tests pass as expected. Could you please review this PR? |
| - name: Generate testing matrix | ||
| id: generate_matrix | ||
| run: | | ||
| DIRS_TO_TEST=$(find dart -name pubspec.yaml -not -path "*/.dart_tool/*" -exec dirname {} \;) |
There was a problem hiding this comment.
This will skip dart files in other parts of the tree. There currently aren't any, but that doesn't mean there couldn't be (imagine a tool implemented in Dart, for instance), and it is a monorepo, so the top level directory is also excluded by this. I'd search the whole tree.
|
Actually, the automation added by this PR is already addressed in the Flutter CI, and presubmit lint/format checks. What is being checked here that isn't already being checked in the other ones? |
|
You're right, and thanks for pushing back on it. #2439 added On the wider point: I'm between jobs at the moment and putting the time into this repo, so I'm glad to take on whatever is actually useful — triage, reproducing issues, filling gaps in test coverage, the unglamorous parts. Just point me at it. |
Description
Fixes #2493, and answers the lint half of #2463.
dart/a2ui_coreanddart/a2ui_agent— the two packages this repository publishes to pub.dev — are not covered by any CI job. The Flutter workflow's matrix comes fromfind samples -name pubspec.yaml, sodart/never enters it; no other workflow runsdart analyzeordart test; andfix_format.shformatssamples/client/flutter renderers/flutter, which does not includedart/either. #2374 changedMessageProcessorand was merged without CI ever running that package's tests.This adds
dart_packages_test.yml, modelled onflutter_packages_test.yml, holding those packages to the same bar as the samples:dart pub get,dart format --output=none --set-exit-if-changed,dart analyze --fatal-infos,dart test. The matrix is generated the same way the Flutter one is, so a new package underdart/is picked up with no workflow edit.Both packages pass all four steps today:
a2ui_core74 tests,a2ui_agent1, and the analyzer is clean in both after the first commit here.Why
--fatal-infos, and why it answers #2463The analyzer reports lint violations as infos. A Dart job without
--fatal-infosexits 0 on a file that breaks every rule inanalysis_options.yaml, so the flag is what makes this a lint gate rather than a compile check. The samples already use it (flutter analyze --fatal-infos).For the question in #2463 — "are we applying the lints that we expect?" — this is the part that was actually missing. The warnings in that issue come from
dart format, which readsanalysis_options.yamlonly for itsformatter:section (page_width) and never for lint rules; this repository sets noformatter:section, so the unresolved include leaves formatting output unchanged. What does change that output is the language version, whichfix_format.shalready pins for exactly that reason. Neither path applies a single lint todart/, because no job analyzes it.Why Flutter and not a Dart-only SDK
The packages use
resolution: workspace, and the rootpubspec.yamldeclaresflutter: ">=3.35.7 <4.0.0"and lists the Flutter samples in its workspace, sodart pub getfails under a Dart-only SDK — the root cause described in #2463. The job therefore uses the repository'ssetup-dartaction, which installs Flutter. If the root pubspec is later split as proposed there, the job can drop to a lightweight Dart SDK by swapping that one step; there is a comment at the step saying so.First commit
dart analyze --fatal-infosneeds the threeunnecessary_library_directiveinfos resolved first. #2439 is already resolving the same three, and I matched its approach rather than inventing a second one: the two libraries ina2ui_coreget the doc comment they were missing,a2ui_agent's bare directive is removed.reactivity.dartanda2ui_agent.dartcome out byte-identical to that PR, so they merge with no conflict;a2ui_core.dartconflicts only in the wording of the doc comment. If #2439 lands first I will rebase and keep its wording — the workflow itself does not conflict with anything.Verification
I cannot run GitHub Actions locally, so I ran the workflow's own steps, including the matrix-generation script verbatim:
The workflow file parses as YAML, carries the license header (
fix_licenses.py --checkdoes not flag it) and is Prettier-clean.A note on the checklist below
As in #2491, I have not touched the packages' CHANGELOG.md files, so the changelog health check will flag this PR. #2439 already adds a
## 0.2.0section to both, and a competing section here would only give that PR a conflict to resolve. The change in this PR is a doc comment and a removed directive — no API or behaviour change — but I am glad to add entries wherever you prefer.Pre-launch Checklist
One time:
For this PR: