Counts go_router and go_router_builder custom test folders as tests #2973
Conversation
| !filename.endsWith('run_tests.sh')) { | ||
| needsTests = !_allChangesAreCodeComments(file); | ||
| } | ||
| // See https://github.com/flutter/flutter/wiki/Plugin-Tests for discussion | ||
| // of various plugin test types and locations. | ||
| if (filename.endsWith('_test.dart') || | ||
| // Test files in custom package-specific test folders. | ||
| filename.contains(RegExp('packages/packages/[^/]+/tool/')) || |
There was a problem hiding this comment.
It's not the case that everything in the tool directory is a test, so this is too broad. tool is just the convention for non-public scripts: https://dart.dev/tools/pub/package-layout#public-tools
E.g., a code generation script might live in tool, but not be a test.
There was a problem hiding this comment.
What about we match any file that has word test anywhere in this folder? something like this
'packages/packages/[^/]+/tool/.+test.+'
Or do you prefer more package specific exception here? like
packages/packages/go_router/tool/test_fixes
There was a problem hiding this comment.
What about we match any file that has word
testanywhere in this folder? something like this
'packages/packages/[^/]+/tool/.+test.+'
There are currently only three examples of that pattern, at at least one is a false positive (pigeon/tool/test.dart, which is just a script for running manual tests locally, so provides no automated test coverage).
The missing-test bot should err on the side of false positives rather than false negatives.
There was a problem hiding this comment.
sounds good I will make this more specific to package instead. If we have more and more use cases, we can come up with a more systematic solution later.
chunhtai
changed the title
chunhtai
changed the title
| @@ -430,6 +430,9 @@ class GithubWebhookSubscription extends SubscriptionHandler { | |||
| // See https://github.com/flutter/flutter/wiki/Plugin-Tests for discussion | |||
| // of various plugin test types and locations. | |||
| if (filename.endsWith('_test.dart') || | |||
| // Test files in package-specific test folders. | |||
| filename.contains('packages/packages/go_router/test_fixes/') || | |||
| filename.contains('packages/packages/go_router_builder/test_inputs/') || | |||
There was a problem hiding this comment.
Nit: Could you put these at the end, so the generic rules continue to be first?
| @@ -430,6 +430,9 @@ class GithubWebhookSubscription extends SubscriptionHandler { | |||
| // See https://github.com/flutter/flutter/wiki/Plugin-Tests for discussion | |||
| // of various plugin test types and locations. | |||
| if (filename.endsWith('_test.dart') || | |||
| // Test files in package-specific test folders. | |||
| filename.contains('packages/packages/go_router/test_fixes/') || | |||
There was a problem hiding this comment.
The packages/packages/ part of these parts unnecessarily ties these patterns to the specific repo structure; we should just start with the package name.
chunhtai
added
the
autosubmit
related flutter/packages#4627
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.