Only run dart files as a test in the Android preview tools test shard #140099
Conversation
dev/bots/test.dart
Outdated
| .listSync(recursive: true).whereType<File>() | ||
| .map<String>((FileSystemEntity entry) => path.relative(entry.path, from: _toolsPath)) | ||
| .toList(); | ||
| .where((String testPath) => path.basename(testPath).endsWith('.dart')).toList(); |
There was a problem hiding this comment.
What about:
| .where((String testPath) => path.basename(testPath).endsWith('.dart')).toList(); | |
| .where((String testPath) => path.basename(testPath).endsWith('_test.dart')).toList(); |
There was a problem hiding this comment.
Would you prefer that filter? For context the change from _runIntegrationToolTests was intentional because the flutter_build_preview_sdk.dart test had historically been excluded from running without anyone noticing, but I don't have a strong preference and can change if you recommend
There was a problem hiding this comment.
since the default behavior of dart test is to only load files ending in _test.dart, by convention we have various helper libraries under //flutter_tools/test that should NOT be run by dart test, such as https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/dap/mocks.dart.
I don't feel too strongly though, it's just if someone wanted to add such a helper in the future, they'd need to update this line.
There was a problem hiding this comment.
To put another way, if there's a test file not ending in _test.dart, I would call that a bug that the test was named wrong, and it wouldn't run locally with an invocation of dart test, and we should fix that, rather than have the CI runner actually run it.
If you wanted to be able to verify that we don't have any test files that don't run on CI because of this, the only way I could think of to do that would be to add a check to //dev/bots/analyze.dart and have an allow-listed of the above-mentioned test helper libraries that are not themselves test entrypoints.
There was a problem hiding this comment.
changed to suggested, I think it is reasonable that people would expect it to work like the one it very clearly based off of regardless
There was a problem hiding this comment.
To put another way, if there's a test file not ending in _test.dart, I would call that a bug that the test was named wrong, and it wouldn't run locally with an invocation of
dart test, and we should fix that, rather than have the CI runner actually run it.If you wanted to be able to verify that we don't have any test files that don't run on CI because of this, the only way I could think of to do that would be to add a check to
//dev/bots/analyze.dartand have an allow-listed of the above-mentioned test helper libraries that are not themselves test entrypoints.
This comment was really helpful for me to get the context. I was one of the ones that encouraged gray to change the implementation.
gmackall
added
the
autosubmit
flutter/flutter@cdc83e5...0eb7881 2023-12-19 [email protected] Shift some Linux-hosted tests on moto g4 to pixel 7 (flutter/flutter#140389) 2023-12-19 [email protected] Reland "Warn when Gradle plugins are applied using the legacy apply script method (flutter/flutter#140103) 2023-12-19 [email protected] Roll Packages from d7dee79 to be52ac8 (1 revision) (flutter/flutter#140386) 2023-12-19 [email protected] Roll Flutter Engine from bde7c612f6ba to 1d5a141917fa (3 revisions) (flutter/flutter#140371) 2023-12-19 [email protected] Roll Flutter Engine from 42af8f106206 to bde7c612f6ba (2 revisions) (flutter/flutter#140367) 2023-12-19 [email protected] Roll Flutter Engine from bbfee6f59b42 to 42af8f106206 (1 revision) (flutter/flutter#140362) 2023-12-19 [email protected] Upgrade to version of leak tracker that does not depend on test_widgets. (flutter/flutter#140247) 2023-12-19 [email protected] Roll Flutter Engine from 632103f787ba to bbfee6f59b42 (4 revisions) (flutter/flutter#140359) 2023-12-18 [email protected] Roll Flutter Engine from 92d88c7e4e9a to 632103f787ba (9 revisions) (flutter/flutter#140350) 2023-12-18 [email protected] SemanticsProperties: default `identifier` and `tooltip` to null (flutter/flutter#140283) 2023-12-18 [email protected] Document difference between softWrap and maxLine (flutter/flutter#139363) 2023-12-18 [email protected] Roll Flutter Engine from 2dc18738167e to 92d88c7e4e9a (1 revision) (flutter/flutter#140332) 2023-12-18 [email protected] Only run dart files as a test in the Android preview tools test shard (flutter/flutter#140099) 2023-12-18 [email protected] Roll Flutter Engine from e4fa972d70cb to 2dc18738167e (1 revision) (flutter/flutter#140328) 2023-12-18 [email protected] Roll Flutter Engine from d7f0803dd6af to e4fa972d70cb (1 revision) (flutter/flutter#140324) 2023-12-18 [email protected] Roll Packages from 3f2e16b to d7dee79 (5 revisions) (flutter/flutter#140322) 2023-12-18 [email protected] Roll Flutter Engine from 7f1a81d5545a to d7f0803dd6af (2 revisions) (flutter/flutter#140320) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
XilaiZhang
added
cp: beta
…flutter#140099) I added a README at the end of flutter#131901 and did not realize that it was being run as a test, [leading to test failures (of course)](https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8761789207346423409/+/u/run_test.dart_for_android_preview_tool_integration_tests_shard_and_subshard_None/test_stdout). This makes it so we only run dart files as dart tests.
Enables the check that was added in flutter#131901. Has been passing since flutter#140099 (30 runs). Not sure what the normal number of successful runs we wait for is before enabling, let me know if we should wait for more data.
4 participants
I added a README at the end of #131901 and did not realize that it was being run as a test, leading to test failures (of course). This makes it so we only run dart files as dart tests.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.