Allow @Ignore to be used on test classes without BUILD file change #4073
Conversation
|
Can one of the admins verify this patch? |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
Currently a test class annotated with `@Ignore` will cause the test runner to fail with ``` Exception in thread "main" java.lang.IllegalArgumentException: Top test must be a suite at com.google.testing.junit.runner.junit4.JUnit4TestModelBuilder.get(JUnit4TestModelBuilder.java:53) ``` This change treats classes with no tests (either no @test annotations or @ignore at class level) as an empty test suite. The main motivation behind this is allowing an entire test class to be ignored (e.g. to quickly deal with a flaky test) without having to modify the BUILD file. This is desirable in order to reduce the likelihood that a developer forgets to update the BUILD file when removing the `@Ignore` annotation.
snowp
force-pushed
the
allow-empty-tests
branch
from
f648b30
to
292a78d
Compare
|
Had some extraneous commits that got dragged in and messed up the googlebot check, but I cleaned that up |
|
@googlebot: ping |
|
The problem with the CLA bot: the email you used to register with the CLA is the one at Square. Since you are covered by the Corporate CLA you should use that one instead. |
|
Ah, that makes sense. I'll reauthor the commit |
| @@ -50,8 +50,9 @@ public JUnit4TestModelBuilder(Request request, @TopLevelSuite String suiteName, | |||
| public TestSuiteModel get() { | |||
| Description root = request.getRunner().getDescription(); | |||
| if (!root.isSuite()) { | |||
There was a problem hiding this comment.
Can you please add a comment explaining the logic of the if branches?
| @@ -50,8 +50,9 @@ public JUnit4TestModelBuilder(Request request, @TopLevelSuite String suiteName, | |||
| public TestSuiteModel get() { | |||
| Description root = request.getRunner().getDescription(); | |||
| if (!root.isSuite()) { | |||
| throw new IllegalArgumentException("Top test must be a suite"); | |||
| return builder.build(suiteName); | |||
There was a problem hiding this comment.
Does this method exist? I cannot find it in the TestSuiteModel
|
What's the status? |
|
@StephenAmar I haven't had the bandwidth to see this one through and probably won't for another few weeks. Feel free to take this over or make your own fix based on mine if this issue is more urgent for you. |
Currently a test class annotated with `@Ignore` will cause the test runner to fail with ``` Exception in thread "main" java.lang.IllegalArgumentException: Top test must be a suite at com.google.testing.junit.runner.junit4.JUnit4TestModelBuilder.get(JUnit4TestModelBuilder.java:53) ``` This change treats classes with no tests (either no @test annotations or @ignore at class level) as an empty test suite. The main motivation behind this is allowing an entire test class to be ignored (e.g. to quickly deal with a flaky test) without having to modify the BUILD file. This is desirable in order to reduce the likelihood that a developer forgets to update the BUILD file when removing the `@Ignore` annotation. This pull request overrides the previous pull request bazelbuild#4073
|
Done with #4293 |
|
Thanks! |
Currently a test class annotated with `@Ignore` will cause the test runner to fail with ``` Exception in thread "main" java.lang.IllegalArgumentException: Top test must be a suite at com.google.testing.junit.runner.junit4.JUnit4TestModelBuilder.get(JUnit4TestModelBuilder.java:53) ``` This change treats classes with no tests (either no @test annotations or @ignore at class level) as an empty test suite. The main motivation behind this is allowing an entire test class to be ignored (e.g. to quickly deal with a flaky test) without having to modify the BUILD file. This is desirable in order to reduce the likelihood that a developer forgets to update the BUILD file when removing the `@Ignore` annotation. This pull request overrides the previous pull request #4073 Closes #4293. PiperOrigin-RevId: 188850828
Currently a test class annotated with `@Ignore` will cause the test
runner to fail with
```
Exception in thread "main" java.lang.IllegalArgumentException: Top test must be a suite
at com.google.testing.junit.runner.junit4.JUnit4TestModelBuilder.get(JUnit4TestModelBuilder.java:53)
```
This change treats classes with no tests (either no @test annotations or
@ignore at class level) as an empty test suite. The main motivation
behind this is allowing an entire test class to be ignored (e.g. to
quickly deal with a flaky test) without having to modify the BUILD file.
This is desirable in order to reduce the likelihood that a developer
forgets to update the BUILD file when removing the `@Ignore` annotation.
This pull request overrides the previous pull request
bazelbuild/bazel#4073
Closes #4293.
PiperOrigin-RevId: 188850828
Currently a test class annotated with
@Ignorewill cause the test runner to fail withThis happens because a test class with
@Ignorewill have 0 tests in theRequestcreated from the test class.This change treats classes with no tests (e.g. no
@Testannotations or@Ignoreat class level) as an empty test suite. The main motivation behind this is allowing an entire test class to be ignored (e.g. to quickly deal with a flaky test) without having to modify the BUILD file. This is desirable in order to reduce the likelihood that a developer forgets to update the BUILD file when removing the@Ignoreannotation.Not sure if this should be flagged off or not? I don't imagine anyone relying on the check that I'm removing, but I am making it a bit more permissive so might be a good idea?