Merge mono_pkg steps to avoid blocking tests with analyze issues

[osa1]

Currently if analyze step fails we don't run tests.

With this we now run tests even when analyze fails.

Analyze step can start to fail without any changes in this repo (e.g. when a new SDK comes with a new deprecation).

However regardless of why analyze fails, it doesn't make sense to stop testing just because there's a warning.

[github-actions]

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:protobuf	4.0.0-dev	ready to publish	protobuf-v4.0.0-dev
package:protoc_plugin	22.0.0-dev	ready to publish	protoc_plugin-v22.0.0-dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

[jakemac53]

However regardless of why analyze fails, it doesn't make sense to stop testing just because there's a warning.

The main reason for this behavior is just to save on CI compute resources - not running the more expensive jobs (tests) when it is known that the overall run will fail anyways.

In the days of Travis, we had a limited number of available jobs as a pool for the whole dart-lang org and so this was more important. It is still kind of nice to do just to be a good steward but I do understand that it can also be annoying to not have the tests run sometimes. Ultimately it is up to you.

[osa1]

The main reason for this behavior is just to save on CI compute resources - not running the more expensive jobs (tests) when it is known that the overall run will fail anyways.

In the days of Travis, we had a limited number of available jobs as a pool for the whole dart-lang org and so this was more important. It is still kind of nice to do just to be a good steward but I do understand that it can also be annoying to not have the tests run sometimes. Ultimately it is up to you.

Thanks for the background.

Since testing packages in this repo are not done by just dart test, often when a contributor sends a PR they don't fully test it. Because the analyze step is often broken (see (2)), this means CI also doesn't test the PR. For example in #911 I had to locally test to find a bug which was caught by the tests, and fix it myself.

Secondly, analyze step gets broken by changes outside of this repo, because it uses the latest SDK available, rather than some fixed SDK version specified in the repo.

With this PR I want to fix (1), but I think (2) is also worth fixing. (we can still have a separate analyze step with the latest SDK, but it should not be reported as "failure" and it should not block anything)

If we fix (2), then (1) becomes less of a problem, but I would argue that it's still a problem and worth fixing, because it costs engineer time when I can't get tests results because e.g. I used single quote instead of double in a string literal.

[jakemac53]

Since testing packages in this repo are not done by just dart test, often when a contributor sends a PR they don't fully test it. Because the analyze step is often broken (see (2)), this means CI also doesn't test the PR. For example in #911 I had to locally test to find a bug which was caught by the tests, and fix it myself.

Fwiw, there is a mono_repo presubmit command which you may be interested in highlighting in a CONTRIBUTING.md or similar. It isn't perfect, won't do any install steps, but will try to run the same tasks as your CI would, in the configurations that match your local one.

Because the analyze step is often broken (see (2)), this means CI also doesn't test the PR. For example in #911 I had to locally test to find a bug which was caught by the tests, and fix it myself.

You could consider splitting out analysis into two jobs, one with --fatal-infos and one without, and allow the --fatal-infos one to fail so it is just an FYI instead. Or, you can choose not to ever pass it, you are signing up for some pain with the combination of --fatal-infos and running the main SDK branch especially.

Presumably most of these failures are just related to info messages not warnings/errors?

[osa1]

Fwiw, there is a mono_repo presubmit command which you may be interested in highlighting in a CONTRIBUTING.md or similar. It isn't perfect, won't do any install steps, but will try to run the same tasks as your CI would, in the configurations that match your local one.

That's great, except I don't think it will work for anyone. Dart 3.3.0 is a supported version, but mono_repo presubmit refuses to test with it:

$ mono_repo presubmit
api_benchmark
  SDK: dev TASK: dart format --output=none --set-exit-if-changed .
    skipped, mismatched sdk
  SDK: dev TASK: ./../tool/setup.sh
    skipped, mismatched sdk
  SDK: dev TASK: ./compile_protos.sh
    skipped, mismatched sdk
  SDK: dev TASK: dart analyze --fatal-infos
    skipped, mismatched sdk
benchmarks
  SDK: dev TASK: dart format --output=none --set-exit-if-changed .
    skipped, mismatched sdk
  SDK: dev TASK: ./../tool/setup.sh
    skipped, mismatched sdk
  SDK: dev TASK: ./tool/compile_protos.sh
    skipped, mismatched sdk
  SDK: dev TASK: dart analyze --fatal-infos
    skipped, mismatched sdk
protobuf
  SDK: dev TASK: dart format --output=none --set-exit-if-changed .
    skipped, mismatched sdk
  SDK: dev TASK: dart analyze --fatal-infos
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: dart analyze lib
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: dart analyze test
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: dart test
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: dart test
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: dart test
    skipped, mismatched sdk
  SDK: dev TASK: dart test
    skipped, mismatched sdk
  SDK: dev TASK: dart test
    skipped, mismatched sdk
  SDK: dev TASK: dart test
    skipped, mismatched sdk
protoc_plugin
  SDK: dev TASK: dart format --output=none --set-exit-if-changed .
    skipped, mismatched sdk
  SDK: dev TASK: ./../tool/setup.sh
    skipped, mismatched sdk
  SDK: dev TASK: make protos
    skipped, mismatched sdk
  SDK: dev TASK: dart analyze --fatal-infos bin lib test
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: ./../tool/setup.sh
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: make protos
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: dart test
    skipped, mismatched sdk
  SDK: dev TASK: ./../tool/setup.sh
    skipped, mismatched sdk
  SDK: dev TASK: make protos
    skipped, mismatched sdk
  SDK: dev TASK: dart test
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: ./../tool/setup.sh
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: make protos
    skipped, mismatched sdk
  SDK: 2.19.0 TASK: dart test legacy_tests/generated_message_test.dart
    skipped, mismatched sdk

It looks like it's looking for the minimum supported SDK version and doesn't test at all if you have another but still supported version. I doubt anyone will contribute patches using SDK 2.19.

Presumably most of these failures are just related to info messages not warnings/errors?

Right now a deprecation warning blocks testsing, which we can't really fix, as 3.3.0 doesn't have the replacement for the deprecated member.

[jakemac53]

Right now a deprecation warning blocks testsing, which we can't really fix, as 3.3.0 doesn't have the replacement for the deprecated member.

You should add an // ignore comment with a TODO and an issue

[jakemac53]

It looks like it's looking for the minimum supported SDK version and doesn't test at all if you have another but still supported version. I doubt anyone will contribute patches using SDK 2.19.

If you were testing on latest stable, it would also run tests though right? It wouldn't be a bad idea to test that as well. But yes this is generally an issue, that it will only run tests that roughly match your actual SDK, because otherwise they might just be invalid.

[osa1]

You should add an // ignore comment with a TODO and an issue

Done in #915.

[devoncarew]

Not sure if you'd like a review from me here? I'm a little less familiar w/ configuring mono_repo.

FWIW, for smaller mono-repos repos (like this one w/ 4 packages), I've gotten pretty far w/ brief, manually written github action configs. They're easy to read + understand for people coming upon them, and straightforward to customize for specific repo / package's needs.

[jakemac53]

FWIW, for smaller mono-repos repos (like this one w/ 4 packages), I've gotten pretty far w/ brief, manually written github action configs. They're easy to read + understand for people coming upon them, and straightforward to customize for specific repo / package's needs.

Just to counterpoint this, mono_repo made the transition from travis ci -> github actions far smoother than it would have been otherwise. So I am a strong proponent of having a layer of abstraction between your actual CI system and how you configure your jobs because of that experience.

I would argue for mono_repo even in a single package repo just for that (when we have dozens of repos especially).