-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Ensure All tests run all the time #2842
Ensure All tests run all the time #2842
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes are great except the ones on listeners which need more discussions.
import spock.lang.Specification | ||
|
||
class SpockSample extends Specification { | ||
|
||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not supposed to annotate a spock test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juherr without the @Test
annotation, JUnit refuses to find the method and crashes with No runnable methods
error. I have tried this same sample and went back as old as https://mvnrepository.com/artifact/org.testng/testng/6.9.13.6 which was released in Sept 2016 and the behavior is still the same. If you know what is causing this problem, please help point to it. But without this annotation the test is not discoverable by JUnit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the point: a Spock test is not a JUnit test
There is a very specific integration https://github.com/cbeust/testng/blob/563bd6dfe2e74475b45110d1cea8101641ebf2b5/testng-core/src/main/java/org/testng/junit/JUnit4SpockMethod.java and the test was here to check the integration.
If the test is failing, it just means the integration is broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juherr - This class was created to deal with test results it looks like. I dont see any code which seems to suggest that it was created to discover tests. I say this because this class is getting instantiated ONLY when we are at the point of creating results.
org.testng.junit.JUnit4TestRunner#createTestResult
> org.testng.junit.JUnit4TestMethod#JUnit4TestMethod
> org.testng.junit.JUnit4TestMethod#getMethod
This is the call stack. I dont see any traces of code that attempts at discovering a Spock test in our codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore all this. You were right. I found the problem. It was version upgrades :) We were trying to work with spock 2.x series which expects the JUnit engine like runner to run but TestNG is not going to be there :) so i had to pin us back to the 1.x series of version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can deprecate the Spock support in the next major release.
In case users have both testng and spock tests, I think the current state of the art is using the junit5 platform with spock and testng flavors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, same remark for the JUnit support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So TestNG 8.0 with the deprecation or we deprecate in 7.7.0 and then get rid of the support for JUnit and spock in 8 ?
7b04d17
to
887632e
Compare
testng-core/src/test/java/test/listeners/github1296/MyConfigurationListener.java
Show resolved
Hide resolved
887632e
to
1ec284c
Compare
@juherr - I have fixed all the comments. Please check now. |
@juherr - Now we have a chicken and egg problem If I upgrade Groovy, then the spock test would fail. If I downgrade Groovy to match the Spock 1.x series, then I see that there are groovy compilation failures in JDK17 See here What do you suggest we do ? We either rework on the spock support to ensure that it is compliant with the spock 2.x series (or) we disable the Spock Test and move on because its anyways a "deprecation eligible feature"
|
upgrading Groovy throws this error on the build
|
I think we have to drop the spock support right now because it is broken since we use jdk17. |
Sure will do that. Can we go ahead with merging this PR if all is well ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge once the method is renamed.
testng-core/src/test/java/test/dataprovider/issue2819/SimpleRetry.java
Outdated
Show resolved
Hide resolved
1ec284c
to
ceccb5a
Compare
Closes #2841
Fixes #2841 .