- 
                Notifications
    You must be signed in to change notification settings 
- Fork 9.1k
HADOOP-19608. Upgrade to JUnit 5.13.3 + Surefire 3.5.3 #7785
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
HADOOP-19608. Upgrade to JUnit 5.13.3 + Surefire 3.5.3 #7785
Conversation
| 💔 -1 overall 
 
 This message was automatically generated. | 
| 💔 -1 overall 
 
 This message was automatically generated. | 
| aws failures (probably unrelated, given I'd not tested trunk yet)  | 
| Surefire rejects me running without a named test  | 
a33729c    to
    ac5300c      
    Compare
  
    | 
 that is a known issue some surefire upgrade maybe did that. We can put that in pom & get rid of this problem Looking at the previous build results, I think the Jenkins didn't run any test, we might need to touch something here & there to trigger all the tests | 
| 💔 -1 overall 
 
 This message was automatically generated. | 
* junit.jupiter and junit.vintage => 5.13.3 * junit.platform => 1.13.3 * Surefire => 3.5.3. Without that tests weren't being found. * cut a duplicate and conflicting import of mockito-jupiter; maven was warning of this.
ac5300c    to
    92484ab      
    Compare
  
    | tested s3 london. lots of Itests failing, which will need fixing after. That test  | 
| 💔 -1 overall 
 
 
 This message was automatically generated. | 
| HDFS tests failing. Of the two classes run in my IDE 
 Assuming unrelated and cutting the hdfs test run trigger | 
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-junit-jupiter</artifactId> | ||
| <version>3.12.4</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.
is this change necessary? or just clean up unused dependency declarations
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.
I was getting a warning of a double reference, and while I was doing jupiter stuff, thought I should do this too
| 💔 -1 overall 
 
 
 This message was automatically generated. | 
Build property trimStackTrace is set to false.
This is restores the behavior that any test failure
includes a deep stack trace rather than the first two elements.
In this version of surefire it also seems to print the full stack
of JUnit test execution and the java runtime itself.
This is only an issue for when tests fail.
Build property surefire.failIfNoSpecifiedTests is set to false.
This is required to retain the behavior where the invocation
    mvn verify -Dtest=unknown
will skip the unit tests but still run the integration tests, and a
specific property "it.test" can name the specific test to run:
  mvn verify -Dtest=unknown -Dit.test=ITestConnection
    c1d727a    to
    ffbf3b0      
    Compare
  
    | Last forced commit squashed all work and cut out the changes to trigger the hdfs and commons builds -just updates the project pom and adds dsocs. can I get this in ASAP, as it is needed for #7814 and any other PR trying to run fs contract tests parameterized. | 
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.
If the build doesn't complains. Changes LGTM
| <value>tests.fake-default.value</value> | ||
| <description>a default value for the "new" key of a deprecated pair.</description> | ||
| </property> | ||
|  | 
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.
i think this is some leftover? if so, do drop it before you commit
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.
yeah, I missed that, and yes, its to be dropped. It'll force hadoop-common tests to run as the final safety check
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| @steveloughran LGTM. After #7814 is completed, I will proceed with fixing the parameterized test issues in other modules. | 
| @slfan1989 thanks, should be good soon, though miniyarn clusters aren't playing (think it's unrelated). And I know a lot more about JUnit 5, even though I think the team don't value developer's existing codebase or knowledge enough. Most unhappy about 
 At least our adoption of AssertJ was prescient. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
…pache#7785) JUnit has been upgraded to 5.13.3 so that test suites can be parameterized at the class level (again). This requires a matching upgrade to Surefire and some tuning of surefire options to restore existing behavior. Dependency Changes: * junit.jupiter and junit.vintage => 5.13.3 * junit.platform => 1.13.3 * Surefire => 3.5.3. Changed Surefire Flags: * trimStackTrace => false * surefire.failIfNoSpecifiedTests => false Build property trimStackTrace ----------------------------- trimStackTrace is set to false by default This restores the behavior that any test failure includes a deep stack trace rather than the first two elements. In this version of surefire it also seems to print the full stack of JUnit test execution and the java runtime itself. This is only an issue for when tests fail. If the short trace is desired, enable it on the command line: mvn test -DtrimStackTrace Build property surefire.failIfNoSpecifiedTests ---------------------------------------------- surefire.failIfNoSpecifiedTests is set to false by default This is required to retain the behavior where the invocation mvn verify -Dtest=unknown will skip the unit tests but still run the integration tests, and a specific property "it.test" can name the specific test to run: mvn verify -Dtest=unknown -Dit.test=ITestConnection Contributed by Steve Loughran (cherry picked from commit d35b321)
HADOOP-19608
How was this patch tested?
Worked on the iceberg bulk delete branch for its new parameterized test class.
Not yet tested anything else ... let's see what yetus says. I'll do the ITest run
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?