Skip to content

Conversation

@mosuem
Copy link
Member

@mosuem mosuem commented Jul 21, 2025

This time using a try-catch instead of just checking the exit code - this didn't seem to work on Dart CI.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@github-actions
Copy link

PR Health

Breaking changes ⚠️
Package Change Current Version New Version Needed Version Looking good?
native_toolchain_c Breaking 0.17.0 0.17.1-wip 0.18.0
Got "0.17.1-wip" expected >= "0.18.0" (breaking changes)
⚠️

This check can be disabled by tagging the PR with skip-breaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/native_doc_dartifier/lib/native_doc_dartifier.dart
pkgs/native_doc_dartifier/lib/src/native_doc_dartifier_base.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@dcharkes
Copy link
Collaborator

We use skips in the dart status files, e.g. https://dart-review.googlesource.com/c/sdk/+/441500

(If we want to skip tests in a different way we should probably do so consistently across the code base. For all missing tools, but then we don't have a code-reviewed list of what is skipped, so we might accidentally start skipping things if bot configurations change. So probably not worth doing a full codebase refactoring for that.)

@dcharkes
Copy link
Collaborator

@goderbauer WDYT? Should we make smarter skips in all our tests so that you can run a subset of the tests locally with dart test without having every single tool installed?

@coveralls
Copy link

Coverage Status

coverage: 75.295%. remained the same
when pulling 17fbea5 on addTryCatch
into cc5539f on main.

@goderbauer
Copy link
Contributor

I prefer using skip over an early return in a test because the skip shows up properly as such in the test log (whereas tests with early return just get marked as passed).

@dcharkes
Copy link
Collaborator

dcharkes commented Jul 21, 2025

I prefer using skip over an early return in a test because the skip shows up properly as such in the test log (whereas tests with early return just get marked as passed).

We all agree on that.

Do you prefer skip over status files? E.g. should we author/code-review what tests are allowed to be skipped in the different locations?

  • I want 0 skips on the GitHub CI. (We can do this with --fatal-skipped But that does mean we need to early return for Windows tests not running on Linux and vice versa.)
  • Do we want to author skips on the Dart CI?
  • For local runs actually skips are nice so the tests can run when not all tools are installed, which @mosuem mentioned.

@goderbauer
Copy link
Contributor

goderbauer commented Jul 21, 2025

Sorry if I misunderstood the question. There wasn't much context on the PR.

I want 0 skips on the GitHub CI.
For local runs actually skips are nice so the tests can run when not all tools are installed

Agreed with both. That would be ideal. The skips should be surfaced on local runs, though, so everyone is aware of them (this automatically happens if the test framework's skip functionality is used).

But that does mean we need to early return for Windows tests not running on Linux and vice versa.

Shouldn't that be handled via @testOn annotations by the testing framework itself without requiring an early return?

Do we want to author skips on the Dart CI?

What do you mean by this? Is this just maintaining the status files, e.g. https://github.com/dart-lang/sdk/blob/main/third_party/pkg/native_toolchain_c.status? Are there any problems/issues here we need to solve?

Do you prefer skip over status files?

If we can just get away with using skips in our repository, I'd prefer that over building and maintaining another system on top of it. As you point out, it would probably mean to disallow skips from happening on CI entirely to avoid that we introduce accidental testing gaps (e.g. by accidentally uninstalling the Rust chain from our bots). However, if it is not feasible to disallow skips on CI we may need status files to achieve that goal (i.e. to make sure the rust toolchain test always runs on CI).

@goderbauer
Copy link
Contributor

@mosuem For context directly related to this PR: What was wrong with how the skip was previously done? Is this just a usability improvement to make the skip visible? Or is there more to it? Can you maybe add some context to the PR description?

@mosuem
Copy link
Member Author

mosuem commented Jul 25, 2025

What was wrong with how the skip was previously done?

I thought checking for the exit code of Process.run is enough, but apparently not, so I added a try-catch clause.

Is this just a usability improvement to make the skip visible?

No, it seems to not skip on Dart CI at least.

Can you maybe add some context to the PR description?

Done :)

@mosuem mosuem changed the title Skip rust_test without Rust [native_toolchain_c] Skip rust_test without Rust Jul 25, 2025
@mosuem mosuem marked this pull request as ready for review July 25, 2025 07:56
@mosuem
Copy link
Member Author

mosuem commented Jul 25, 2025

Should we make smarter skips in all our tests

I think we can start with one test, and see how it goes?

@dcharkes
Copy link
Collaborator

I think we haven't answered the conceptual question:

  • Do we want to auto-skip on our CI.

We cannot merge the PR in this current state because it ignores this question.

At a minimum, this PR should add the --fatal-skipped argument to the tool/ci.dart dart test invocation.

On the Dart CI we don't use package:test to drive the tests, so we cannot do that trick there. So adopting these conditional skips will make it so that if we uninstall some tool on the Dart CI test will start to be silently skipped. Maybe that is okay as we mostly run the tests of these packages on the Dart CI to not have dartdev build hook tests fail with not finding the C compiler and having to have to dig through the logs to do so.

I am still not convinced that auto-skipping is a good idea.

Maybe we should use dart test tags instead: https://pub.dev/packages/test#tagging-tests

We can tag tests by cross-compilation, rust, etc. Then you can run dart test -x ... to exclude a subset of tests which requires certain tools to be installed on the system.

Let's discuss our testing strategy in the sync instead of on this PR.

@mosuem
Copy link
Member Author

mosuem commented Jul 25, 2025

We cannot merge the PR in this current state because it ignores this question.

It answers the question ;)

Closing this PR for now then.

@mosuem mosuem closed this Jul 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants