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.

Sign up for GitHub

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

Jump to bottom

Counts go_router and go_router_builder custom test folders as tests #2973

Merged
merged 4 commits into from
Aug 3, 2023
Merged

Counts go_router and go_router_builder custom test folders as tests #2973

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34371a8
Counts custom package specific test runners folder as tests
chunhtai Aug 2, 2023
395c437
update
chunhtai Aug 2, 2023
ce0bd46
revert
chunhtai Aug 2, 2023
95b6416
update
chunhtai Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app_dart/lib/src/request_handlers/github/webhook_subscription.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,13 +423,14 @@ class GithubWebhookSubscription extends SubscriptionHandler {
// for the purposes of testing a change that otherwise needs tests,
// but since they are the driver for tests they don't need test
// coverage.
!filename.endsWith('tool/run_tests.dart') &&
!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/')) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@chunhtai chunhtai Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

What about we match any file that has word test anywhere 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// Native iOS/macOS tests.
filename.contains('RunnerTests/') ||
filename.contains('RunnerUITests/') ||
Expand Down
33 changes: 33 additions & 0 deletions app_dart/test/request_handlers/github/webhook_subscription_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1692,13 +1692,15 @@ void foo() {
),
);
});

test('Packages does not comment if Pigeon native tests', () async {
const int issueNumber = 123;

tester.message = generateGithubWebhookMessage(
action: 'opened',
number: issueNumber,
slug: Config.packagesSlug,
baseRef: Config.defaultBranch(Config.packagesSlug),
);
when(pullRequestsService.listFiles(Config.packagesSlug, issueNumber)).thenAnswer(
(_) => Stream<PullRequestFile>.fromIterable(<PullRequestFile>[
Expand All @@ -1719,6 +1721,37 @@ void foo() {
);
});

test('Packages does not comment if editing test files in tool/', () async {
const int issueNumber = 123;

tester.message = generateGithubWebhookMessage(
action: 'opened',
number: issueNumber,
slug: Config.packagesSlug,
baseRef: Config.defaultBranch(Config.packagesSlug),
);
when(pullRequestsService.listFiles(Config.packagesSlug, issueNumber)).thenAnswer(
(_) => Stream<PullRequestFile>.fromIterable(<PullRequestFile>[
PullRequestFile()
..filename = 'packages/packages/go_router/tool/test_fixes/go_router.dart'
..additionsCount = 10,
PullRequestFile()
..filename = 'packages/packages/go_router/lib/fix_data.yaml'
..additionsCount = 10,
]),
);

await tester.post(webhook);

verifyNever(
issuesService.createComment(
Config.packagesSlug,
issueNumber,
argThat(contains(config.missingTestsPullRequestMessageValue)),
),
);
});

test('Packages comments and labels if no tests', () async {
const int issueNumber = 123;

Expand Down