Skip to content

Conversation

RohitSaily
Copy link
Contributor

@RohitSaily RohitSaily commented Sep 15, 2025

if a test class defines setUpClass it will be run before any tests are ever run. if it is asynchronous then no test runs until the future is completed

tearDownClass is called once after all tests are run

the same InstanceMirror is used for the setup and for the tear-down, following how setup and tear-down is done for each unit test

tests are updated to support and check for new behavior

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for setUpClass and tearDownClass methods, which run once before and after all tests in a class, respectively. The implementation correctly uses a single instance for these class-level methods and ensures setUpClass is only executed once. The accompanying tests effectively verify this new behavior. My feedback focuses on improving documentation and code style for the newly introduced methods to align with the repository's style guide, specifically regarding documenting public members and optimizing for readability.

@RohitSaily RohitSaily force-pushed the classwide branch 2 times, most recently from 8a1598a to d4d0d80 Compare September 15, 2025 22:59
@RohitSaily RohitSaily force-pushed the classwide branch 2 times, most recently from d2ede91 to 71dee67 Compare September 16, 2025 18:20
@RohitSaily RohitSaily force-pushed the classwide branch 7 times, most recently from ee94d1e to 35a07f9 Compare September 17, 2025 18:40
@RohitSaily RohitSaily force-pushed the classwide branch 2 times, most recently from 786294f to 1bc4e97 Compare September 17, 2025 19:46
@RohitSaily
Copy link
Contributor Author

@scheglov i'm not sure what standard protocol is or if it matters, but since your reviews have significantly helped shape the code and its correctness i was thinking you should be credited as an author as well. i just don't know how to add you as one

@scheglov
Copy link
Contributor

@scheglov i'm not sure what standard protocol is or if it matters

Don't worry about this, without you this change would not happen at all :-)

@scheglov
Copy link
Contributor

scheglov commented Sep 17, 2025

About failing test, note that we have special case for other methods: https://github.com/dart-lang/sdk/blob/fcb1e9da9dd4a92c40b55644a7a535ad0ee48d28/pkg/linter/lib/src/rules/unreachable_from_main.dart#L129

Copy link

github-actions bot commented Sep 17, 2025

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
test_reflective_loader None 0.3.0 0.4.0 0.3.0 ✔️

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.

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

Coverage ✔️
File Coverage
pkgs/test_reflective_loader/lib/test_reflective_loader.dart 💚 86 % ⬆️ 2 %

This check for test coverage is informational (issues shown here will not fail the PR).

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

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 symbol Leaking sources

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

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/bazel_worker/benchmark/benchmark.dart
pkgs/bazel_worker/example/client.dart
pkgs/bazel_worker/example/worker.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/coverage/lib/src/coverage_options.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/html/test/trie_test.dart
pkgs/html/tool/generate_trie.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

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

@RohitSaily
Copy link
Contributor Author

About failing test, note that we have special case for other methods: https://github.com/dart-lang/sdk/blob/fcb1e9da9dd4a92c40b55644a7a535ad0ee48d28/pkg/linter/lib/src/rules/unreachable_from_main.dart#L129

thanks for the direct link, i will update that code to consider the newly introduced names

RohitSaily added a commit to RohitSaily/-dart-lang.sdk that referenced this pull request Sep 17, 2025
The PR dart-lang/tools#2164 introduces the ability to define
`setUpClass` and `tearDownClass` static methods in test
classes. Currently, such methods are linted as `unreachable_from_main`
when they should not be linted just like other test methods
@RohitSaily
Copy link
Contributor Author

the unreachable_from_main lint update has been merged: dart-lang/sdk#61528

Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
test_reflective_loader None 0.3.0 0.3.0 0.3.0 ✔️

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

Changelog Entry
Package Changed Files
package:test_reflective_loader pkgs/test_reflective_loader/lib/test_reflective_loader.dart

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

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

Coverage ✔️
File Coverage
pkgs/test_reflective_loader/lib/test_reflective_loader.dart 💚 86 % ⬆️ 2 %

This check for test coverage is informational (issues shown here will not fail the PR).

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

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 symbol Leaking sources

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

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/bazel_worker/benchmark/benchmark.dart
pkgs/bazel_worker/example/client.dart
pkgs/bazel_worker/example/worker.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/coverage/lib/src/coverage_options.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/html/test/trie_test.dart
pkgs/html/tool/generate_trie.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

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

@scheglov
Copy link
Contributor

This was fast!

Unfortunately there are still lints reported, and I suspect that it will happen on older SDKs anyway.
So, lets add ignore annotations for now, with TODO to get back to it after linter changes percolate.
So, we will able to land this PR.

@RohitSaily RohitSaily force-pushed the classwide branch 3 times, most recently from d545fbb to c638442 Compare September 18, 2025 18:21
@RohitSaily
Copy link
Contributor Author

i have added the ignores and todos as you suggested

@RohitSaily
Copy link
Contributor Author

i see the failed checks, i will work on resolving them

@RohitSaily RohitSaily force-pushed the classwide branch 2 times, most recently from cdd779a to 74a6341 Compare September 18, 2025 18:59
@RohitSaily
Copy link
Contributor Author

i made sure there's no warnings now (besides the todos) and updated the change log. i bumped the version to 0.4.0 in the pubspec. everything should be ok now

Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:bazel_worker 1.1.3 already published at pub.dev
package:benchmark_harness 2.4.0-wip WIP (no publish necessary)
package:boolean_selector 2.1.2 already published at pub.dev
package:browser_launcher 1.1.3 already published at pub.dev
package:cli_config 0.2.1-wip WIP (no publish necessary)
package:cli_util 0.5.0-wip WIP (no publish necessary)
package:clock 1.1.3-wip WIP (no publish necessary)
package:code_builder 4.11.0 already published at pub.dev
package:coverage 1.15.0 already published at pub.dev
package:csslib 1.0.2 already published at pub.dev
package:extension_discovery 2.1.0 already published at pub.dev
package:file 7.0.2-wip WIP (no publish necessary)
package:file_testing 3.1.0-wip WIP (no publish necessary)
package:glob 2.1.3 already published at pub.dev
package:graphs 2.3.3-wip WIP (no publish necessary)
package:html 0.15.6 already published at pub.dev
package:io 1.1.0-wip WIP (no publish necessary)
package:json_rpc_2 4.0.0 already published at pub.dev
package:markdown 7.3.1-wip WIP (no publish necessary)
package:mime 2.0.0 already published at pub.dev
package:oauth2 2.0.4-wip WIP (no publish necessary)
package:package_config 2.3.0-wip WIP (no publish necessary)
package:pool 1.5.2 already published at pub.dev
package:process 5.0.5 already published at pub.dev
package:pub_semver 2.2.0 already published at pub.dev
package:pubspec_parse 1.5.0 already published at pub.dev
package:source_map_stack_trace 2.1.3-wip WIP (no publish necessary)
package:source_maps 0.10.14-wip WIP (no publish necessary)
package:source_span 1.10.1 already published at pub.dev
package:sse 4.1.8 already published at pub.dev
package:stack_trace 1.12.1 already published at pub.dev
package:stream_channel 2.1.4 already published at pub.dev
package:stream_transform 2.1.2-wip WIP (no publish necessary)
package:string_scanner 1.4.1 already published at pub.dev
package:term_glyph 1.2.3-wip WIP (no publish necessary)
package:test_reflective_loader 0.4.0 ready to publish test_reflective_loader-v0.4.0
package:timing 1.0.2 already published at pub.dev
package:unified_analytics 8.0.5 already published at pub.dev
package:watcher 1.1.3 already published at pub.dev
package:yaml 3.1.3 already published at pub.dev
package:yaml_edit 2.2.2 already published at pub.dev

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

if a test class defines `setUpClass` it will be run before any
tests are ever run. if it is asynchronous then no test runs
until the future is completed

`tearDownClass` is called once after all tests are run

tests are updated to support and check for new behavior
@RohitSaily
Copy link
Contributor Author

forgot to run dart format 😅 fixed that now

Copy link
Contributor

@scheglov scheglov left a comment

Choose a reason for hiding this comment

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

Thank you!

@scheglov scheglov merged commit 6c1eb21 into dart-lang:main Sep 18, 2025
13 checks passed
@RohitSaily
Copy link
Contributor Author

no problem and thank you for being very responsive, it was a good learning experience for me

i look forward to contributing more where i can

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Sep 22, 2025
Revisions updated by `dart tools/rev_sdk_deps.dart`.

http (https://github.com/dart-lang/http/compare/d8e6929..d6dd5ec):
  d6dd5ec  2025-09-22  Willow Chargin  [http2] Gracefully receive headers on canceled streams (dart-lang/http#1800)

i18n (https://github.com/dart-lang/i18n/compare/a62bed2..09627d2):
  09627d28  2025-09-19  Moritz  Copybara import of the project:

tools (https://github.com/dart-lang/tools/compare/042b03e..6c1eb21):
  6c1eb214  2025-09-18  RohitSaily  add support for `setUpClass` and `tearDownClass` (dart-lang/tools#2164)

Change-Id: I892c69fbdabd650f91845abde6f21fe0866b9e12
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/450940
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
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.

2 participants