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

Add error for empty groups #2012

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft

Add error for empty groups #2012

wants to merge 16 commits into from

Conversation

natebosch
Copy link
Member

Closes #1961

After declaring a group check for declared tests, if there were none add
a synthetic test that fails with an error about the empty group.

Closes #1961

After declaring a group check for declared tests, if there were none add
a synthetic test that fails with an error about the empty group.
@natebosch
Copy link
Member Author

This does cause some failures in existing tests.

For instance: https://github.com/flutter/packages/blob/dfcb21c087d3b7781365e915ad2864b4856c3a41/packages/flutter_markdown/test/emphasis_test.dart#L4393

@jakemac53 - Do you think we can get away with fixing the google3 impacted tests and releasing this as a non-breaking change?

@natebosch
Copy link
Member Author

I found dart-lang/yaml#145 with this change.

@jakemac53
Copy link
Contributor

I found dart-lang/yaml#145 with this change.

Cases like this are actually slightly concerning to me - although they should have been using skip anyways. But if you are commenting out tests one at a time, at some point you then have to comment out the entire group, and that experience is a bit weird/confusing potentially. Given we have skip though which doesn't have this problem, I think it's OK.

The other thing I was thinking about is when people are creating tests programmatically. You could define a group in an outer loop, and then end up skipping all values in an inner loop, thus not adding any tests to it. If that is an error it becomes potentially quite a pain.

@jakemac53
Copy link
Contributor

If we don't have evidence of the programmatic tests being an issue today I think it is probably fine though.

@jakemac53
Copy link
Contributor

@jakemac53 - Do you think we can get away with fixing the google3 impacted tests and releasing this as a non-breaking change?

How many failures are we talking about?

@natebosch
Copy link
Member Author

How many failures are we talking about?

~150

I looked at a random sample of 10 of them. Three were the programmatic case - a group with an if block that either defines or omits a test - they were all in the same directory using the same utility to define tests. The if could be moved to be around the group, but there are other cases in the utility where a group has some conditional and some unconditional tests.

One was a clear mistake, there was a group with a body that should have been a setUp, and then outside of it a single test. It happens to work because the group body definitely runs before the test body, and there is only 1 test.

Five were likely mistakes - they had a setUp but no test. Unlike the original issue no test call in setUp. It could be that the group should have tests, or the group should be removed.

One was a group that had a // TODO.

@jakemac53
Copy link
Contributor

Would it be reasonable to have an allowEmpty parameter on group to help mitigate the programmatic cases?

@natebosch
Copy link
Member Author

Would it be reasonable to have an allowEmpty parameter on group

I could take a look at how much work it would be to implement.

The output already has special behavior for when there are no tests at
all, and the output is less clear when it's the root group which is
empty since the root group has no name.

Also refactor duplicate name tests to have different file content when
testing identical group names to avoid empty groups in any case.
@natebosch
Copy link
Member Author

cc @christopherfujino - This is a technically breaking change but we plan on rolling this out without a major version bump. This will also impact the behavior of the group API exported through flutter_test. It will be possible for the flutter team to decide to not make the breaking change by always passing allowEmpty: true when forwarding group calls. It will also be possible to roll it out as breaking and expose the allowEmpty argument through the forwarding API.

natebosch added a commit to flutter/packages that referenced this pull request Oct 6, 2023
The empty group will soon become a warning or an error. Eagerly skip the
group to avoid any issue when the test runner changes behavior.
dart-lang/test#2012

Note that the linked issue has been resolved, and the comment in this
group body may be out of date.

This change will impact the output from running tests - there will be an
indication of the skipped test.
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 9, 2023
The empty group will soon become a warning or an error. Eagerly skip the
group to avoid any issue when the test runner changes behavior.
dart-lang/test#2012

Note that the linked issue has been resolved, and the comment in this
group body may be out of date.

This change will impact the output from running tests - there will be an
indication of the skipped test.
@natebosch
Copy link
Member Author

I took a look at fixing all the cases in google3, and even the programatic ones were not too difficult to correct, even without an allowEmptyGroup argument. I'm now leaning towards omitting that.

@mosuem also had a good suggestion in a meeting today to only add the error when there is a setUp but no test cases. The main mistake we hope to help catch is when the test are inside the setUp, but if the group is entirely empty it's more likely to be intentional, perhaps with a // TODO comment in the body.

@jakemac53 - WDYT about allowing completely empty groups, but making an error for the case where there is setUp or tearDown with no test.

@jakemac53
Copy link
Contributor

@jakemac53 - WDYT about allowing completely empty groups, but making an error for the case where there is setUp or tearDown with no test.

I would be fine with that

@natebosch
Copy link
Member Author

I found at least one pattern for a mistake which the looser restriction lets through:

group('some group', defineTests(someConstConfig));
group('another group', () {
  var config = someComputedConfig();
  defineTests(config);
});

no setUp, but the second group has no tests. It should have used defineTests(config)(); because that utility was apparently designed to return the body argument to group, and the returned callback must be invoked.

There are quite a few which still need the skip because they do define a setUp, but I do see enough // TODO implement test cases that this helps, and it's a good indication that being looser will be more comfortable while editing.

@jakemac53
Copy link
Contributor

no setUp, but the second group has no tests. It should have used defineTests(config)(); because that utility was apparently designed to return the body argument to group, and the returned callback must be invoked.

Wow, that is a nasty API 🤣 . I don't know if I care enough about that to make the TODO case be more noisy.

HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
The empty group will soon become a warning or an error. Eagerly skip the
group to avoid any issue when the test runner changes behavior.
dart-lang/test#2012

Note that the linked issue has been resolved, and the comment in this
group body may be out of date.

This change will impact the output from running tests - there will be an
indication of the skipped test.
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.

Warn when tests are accidentally disabled due to nesting within setUp() method
2 participants