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

Fix order-dependent test: test the correct instance(s). #3844

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

davidmorgan
Copy link
Contributor

No description provided.

@davidmorgan davidmorgan requested a review from jensjoha February 11, 2025 08:42
Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:build 2.4.2 already published at pub.dev
package:build_config 1.1.2 already published at pub.dev
package:build_daemon 4.0.4 ready to publish build_daemon-v4.0.4
package:build_modules 5.0.11 already published at pub.dev
package:build_resolvers 2.4.4-wip WIP (no publish necessary)
package:build_runner 2.4.15-wip WIP (no publish necessary)
package:build_runner_core 8.0.1-dev ready to publish build_runner_core-v8.0.1-dev
package:build_test 2.2.4-wip WIP (no publish necessary)
package:build_web_compilers 4.1.1 already published at pub.dev
package:scratch_space 1.0.3-wip WIP (no publish necessary)

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

Copy link

PR Health

Changelog Entry ✔️
Package Changed Files

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

@@ -738,36 +738,37 @@ int? get x => 1;
}
''',
}, (resolver) async {

Choose a reason for hiding this comment

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

Does this still have to be async? Or should the calls actually be await resolver.libraryFor as below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, reverted that attempt at a fix.

I realized both these tests were missing resolvers: createResolvers(), which means all four test variants were testing the same shared instance. So, that's the correct fix--done. Thanks!

test('are reported for part files with errors', () {
return resolveSources({
test('are reported for part files with errors', () async {
await resolveSources({
'a|lib.dart': '''
library a_library;
part 'errors.dart';

Choose a reason for hiding this comment

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

I can seemingly not put comments on lines further down --- but why does these in particular have to change when stuff further down looks to be done the same way as these were before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, GitHub doesn't let you comment on unchanged lines--an annoying limitation.

The tests were actually correct, reverted, added resolvers: createResolvers() so we test the right instance, which is now I hope the correct fix :) thanks!

@davidmorgan davidmorgan changed the title Fix order-dependent test: finish testing during the test. Fix order-dependent test: test the correct instance(s). Feb 11, 2025
@davidmorgan davidmorgan merged commit 056e7d6 into dart-lang:master Feb 11, 2025
75 checks passed
@davidmorgan davidmorgan deleted the fix-test branch February 11, 2025 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants