Skip to content

Fix an issue with failed assertions using setUpAll/tearDownAll #2499

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

Merged
merged 3 commits into from
May 19, 2025

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented May 15, 2025

When groups are filtered using filter() and not onPlatform, we would clone the group and tests without also cloning setUpAll/tearDownAll, which led to an assertion failure when trying to set the parents.

This change ensures we clone tests in filter() like we do in forPlatform().

Fixes #2498

When groups are filtered using `filter()` and not `onPlatform`, we would clone the group and tests without also cloning `setUpAll`/`tearDownAll`, which led to an assertion failure when trying to set the parents.

This change ensures we clone tests in `filter()` like we do in `forPlatform()`.

Fixes dart-lang#2498
@DanTup DanTup requested a review from a team as a code owner May 15, 2025 18:08
Copy link
Contributor

@jakemac53 jakemac53 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go ahead and update the other package versions for a release as well.

Copy link
Contributor Author

@DanTup DanTup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -95,4 +95,19 @@ void main() {
expect(test.stdout, neverEmits(contains('(setUpAll)')));
await test.shouldExit(0);
});

test('does not fail with asserts enabled - issue #2498', () async {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why no existing tests triggered this, but did trigger when we didn't have the fix in onPlatform, but I added specific tests for these referencing the issue.

@DanTup
Copy link
Contributor Author

DanTup commented May 15, 2025

@jakemac53

Go ahead and update the other package versions for a release as well.

Did you mean remove the -wips? If so, done - if not, let me know what :)

@DanTup DanTup mentioned this pull request May 15, 2025
Copy link

PR Health

Changelog Entry ✔️
Package Changed Files

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

@DanTup
Copy link
Contributor Author

DanTup commented May 19, 2025

@jakemac53 I don't understand the Publish bot failure - the only messages I can see are about widening the version constraints, but all I did was remove -wip so I'm not sure what the problem is.

@jakemac53
Copy link
Contributor

@jakemac53 I don't understand the Publish bot failure - the only messages I can see are about widening the version constraints, but all I did was remove -wip so I'm not sure what the problem is.

This is expected, I will add the label to skip the check. Pub always warns about pinning deps - actually in this case though it should warn if we weren't pinning deps since we export those packages. But, it doesn't understand that.

@DanTup
Copy link
Contributor Author

DanTup commented May 19, 2025

Got it, thanks!

GitHub says there are workflows needing approval, although I', not sure what since everything seems to have a green tick. I think everything is done here, but let me know if you think there's anything outstanding I need to do.

Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:checks 0.3.1-wip WIP (no publish necessary)
package:fake_async 1.3.3 already published at pub.dev
package:matcher 0.12.18-wip WIP (no publish necessary)
package:test 1.26.1 ready to publish test-v1.26.1
package:test_api 0.7.6 ready to publish test_api-v0.7.6
package:test_core 0.6.10 ready to publish test_core-v0.6.10
package:test_descriptor 2.0.2 already published at pub.dev
package:test_process 2.1.1 already published at pub.dev

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

@jakemac53 jakemac53 merged commit 5ffcb36 into dart-lang:master May 19, 2025
64 of 65 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request May 19, 2025
Revisions updated by `dart tools/rev_sdk_deps.dart`.

protobuf (https://github.com/dart-lang/protobuf/compare/b7753f6..deda288):
  deda288  2025-05-19  Devon Carew  address a merge issue with the protoc_plugin changelog (google/protobuf.dart#1002)
  5ed4611  2025-05-19  Devon Carew  shorten the deprecation messages for clone() and copyWith() (google/protobuf.dart#999)
  4960b92  2025-05-19  Ömer Sinan Ağacan  Switch to pub workspace (google/protobuf.dart#1001)

test (https://github.com/dart-lang/test/compare/b9c59ea..5ffcb36):
  5ffcb36f  2025-05-19  Danny Tuppeny  Fix an issue with failed assertions using setUpAll/tearDownAll (dart-lang/test#2499)

Change-Id: Idc43e2f2fd9dc38b80917b25b45bb954076dc1c9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/429480
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Devon Carew <[email protected]>
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.

Failed assertions when
2 participants