Sitelet https://web.archive.org/web/20210722225347/https://github.com/flutter/flutter/pull/86397
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

Added 'exclude' parameter to 'testWidgets()'. #86397

Merged
merged 2 commits into from Jul 22, 2021

Conversation

@darrenaustin
Copy link
Contributor

@darrenaustin darrenaustin commented Jul 14, 2021

Part of #86396.

Adds a new exclude parameter to testWidgets that behaves the same as skip but is used to mark a test that should always be skipped under the condition as it isn't designed that way. skip is still used to mark tests that are just temporarily shut off to keep the tree open.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.
@skia-gold
Copy link

@skia-gold skia-gold commented Jul 14, 2021

Gold has detected about 1 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/86397

@Hixie
Copy link
Member

@Hixie Hixie commented Jul 14, 2021

Can't one just not call the testWidgets method when the condition is true?
Or return early from the test if that condition is true?

This seems to just add API surface without actually providing more power. Given the complexity of the test API surface I'm very wary of adding more features here. I'd rather dramatically reduce the scope of this API than grow it.

@darrenaustin
Copy link
Contributor Author

@darrenaustin darrenaustin commented Jul 20, 2021

Can't one just not call the testWidgets method when the condition is true?

We could, but it would mean that those tests aren't reported in any way if they are run under the condition that caused them to be skipped. This may be fine, but could lead to some confusion about the results.

(Also, I am not that familiar with the underlying test runner, but @gspencergoog just added the ability to randomly shuffle the order of tests. In this case it would just not add them to the list that is shuffled, right Greg? or would this introduce an issue here?)

Or return early from the test if that condition is true?

This would mean that the test is reported as a success when it didn't run.

This seems to just add API surface without actually providing more power. Given the complexity of the test API surface I'm very wary of adding more features here. I'd rather dramatically reduce the scope of this API than grow it.

While I understand not wanting to grow the API surface, this seems like a good tradeoff to me. It would give a declarative way to say that a test is intentionally skipped under some condition.

@darrenaustin darrenaustin force-pushed the darrenaustin:skip_exclude branch from afe3fad to 5da149e Jul 20, 2021
@skia-gold
Copy link

@skia-gold skia-gold commented Jul 20, 2021

Gold has detected about 5 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/86397

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Jul 20, 2021

(Also, I am not that familiar with the underlying test runner, but @gspencergoog just added the ability to randomly shuffle the order of tests. In this case it would just not add them to the list that is shuffled, right Greg? or would this introduce an issue here?)

I actually had to revert that change, but I will reintroduce it when the test package is next released. In any case, it would just mean that the test wouldn't be included in the shuffled list, so no issue here.

@Hixie
Copy link
Member

@Hixie Hixie commented Jul 20, 2021

I really don't see the problem with reporting the test as successful if it didn't run, or not reporting them when they're skipped, to be honest. Does anyone really look at the precise numbers such that it would matter?

Honestly I'd be perfectly fine with the entire test system being pass/fail, no numbers at all. I wish we could get rid of the test function entirely, use assert instead of expect, etc.

@darrenaustin darrenaustin force-pushed the darrenaustin:skip_exclude branch from 5da149e to cd3d059 Jul 21, 2021
@darrenaustin darrenaustin requested a review from HansMuller Jul 22, 2021
@darrenaustin
Copy link
Contributor Author

@darrenaustin darrenaustin commented Jul 22, 2021

Honestly I'd be perfectly fine with the entire test system being pass/fail, no numbers at all. I wish we could get rid of the test function entirely, use assert instead of expect, etc.

Ah, I can see why you wouldn't be keen on this idea then 😄.

While I respect that, I still see value in having structured tests with data that describe them to making it easier for maintenance and tooling. And if we are going to have this test structure it would be nice to have a way to tell intentional skips from temporary using the existing structure.

@Hixie
Copy link
Member

@Hixie Hixie commented Jul 22, 2021

I hear what you're saying. Is there a way we can add this feature to our own test infrastructure without adding the complexity to our API? I'm worried about how every new person learning how to use tests in Flutter is going to have to distinguish between skip and exclude and not really be able to understand the subtle distinction we're making here. If the goal is just to make skip in some cases not get counted against our benchmarks, is there some other way we can exclude tests from that benchmark?

(At the end of the day, I'm not too worried about us having a few dollars extra on our benchmark, I have to be honest. If that's the worst thing we're dealing with, we can call the project finished and move on to something else. :-) )

Copy link
Contributor

@HansMuller HansMuller left a comment

Skip vs exclude sounds like a worthwhile distinction, LGTM

@darrenaustin darrenaustin merged commit eb62bce into flutter:master Jul 22, 2021
53 checks passed
53 checks passed
@flutter-dashboard
Linux analyze
Details
@flutter-dashboard
Linux build_tests_1_2
Details
@flutter-dashboard
Linux build_tests_2_2
Details
@flutter-dashboard
Linux customer_testing
Details
@flutter-dashboard
Linux docs
Details
@flutter-dashboard
Linux firebase_abstract_method_smoke_test
Details
@flutter-dashboard
Linux firebase_android_embedding_v2_smoke_test
Details
@flutter-dashboard
Linux firebase_release_smoke_test
Details
@flutter-dashboard
Linux flutter_plugins
Details
@flutter-dashboard
Linux framework_tests_libraries
Details
@flutter-dashboard
Linux framework_tests_misc
Details
@flutter-dashboard
Linux framework_tests_widgets
Details
@flutter-dashboard
Linux fuchsia_precache
Details
@flutter-dashboard
Linux web_long_running_tests_1_5
Details
@flutter-dashboard
Linux web_long_running_tests_2_5
Details
@flutter-dashboard
Linux web_long_running_tests_3_5
Details
@flutter-dashboard
Linux web_long_running_tests_4_5
Details
@flutter-dashboard
Linux web_long_running_tests_5_5
Details
@flutter-dashboard
Linux web_tests_0
Details
@flutter-dashboard
Linux web_tests_1
Details
@flutter-dashboard
Linux web_tests_2
Details
@flutter-dashboard
Linux web_tests_3
Details
@flutter-dashboard
Linux web_tests_4
Details
@flutter-dashboard
Linux web_tests_5
Details
@flutter-dashboard
Linux web_tests_6
Details
@flutter-dashboard
Linux web_tests_7_last
Details
@flutter-dashboard
Mac build_tests_1_4
Details
@flutter-dashboard
Mac build_tests_2_4
Details
@flutter-dashboard
Mac build_tests_3_4
Details
@flutter-dashboard
Mac build_tests_4_4
Details
@flutter-dashboard
Mac customer_testing
Details
@flutter-dashboard
Mac framework_tests_libraries
Details
@flutter-dashboard
Mac framework_tests_misc
Details
@flutter-dashboard
Mac framework_tests_widgets
Details
@wip
WIP Ready for review
Details
@flutter-dashboard
Windows build_tests_1_3
Details
@flutter-dashboard
Windows build_tests_2_3
Details
@flutter-dashboard
Windows build_tests_3_3
Details
@flutter-dashboard
Windows customer_testing
Details
@flutter-dashboard
Windows framework_tests_libraries
Details
@flutter-dashboard
Windows framework_tests_misc
Details
@flutter-dashboard
Windows framework_tests_widgets
Details
@cirrus-ci
analyze-linux Task Summary
Details
@flutter-dashboard
ci.yaml validation .ci.yaml validation
Details
@google-cla
cla/google All necessary CLAs are signed
@cirrus-ci
customer_testing-linux Task Summary
Details
@cirrus-ci
docs-linux Task Summary
Details
@flutter-dashboard
flutter-gold All golden file tests have passed.
Details
@cirrus-ci
framework_tests-libraries-linux Task Summary
Details
@cirrus-ci
framework_tests-misc-linux Task Summary
Details
@cirrus-ci
framework_tests-widgets-linux Task Summary
Details
@flutter-dashboard
luci-flutter
Details
@cirrus-ci
web_smoke_test Task Summary
Details
@darrenaustin darrenaustin deleted the darrenaustin:skip_exclude branch Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants