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

Migrate many tests from JUnit 4 to JUnit 5 #1293

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

liblit
Copy link
Contributor

@liblit liblit commented Jul 23, 2023

Most of these migrations were mechanized using IntelliJ IDEA followed by some manual cleanups. A few were migrated entirely manually, such as the expected-to-fail tests in FieldBasedComparisonTest.

@liblit liblit self-assigned this Jul 23, 2023
@liblit liblit force-pushed the junit-5/migrate-many-tests branch 5 times, most recently from 91e36f4 to 426423f Compare July 24, 2023 23:38
Most of these migrations were mechanized using IntelliJ IDEA followed by
some manual cleanups.  A few were migrated entirely manually, such as
the expected-to-fail tests in `FieldBasedComparisonTest`.
@liblit liblit force-pushed the junit-5/migrate-many-tests branch from a800cc4 to c68ae74 Compare July 26, 2023 00:58
@liblit liblit requested a review from msridhar July 26, 2023 01:03
@liblit liblit marked this pull request as ready for review July 26, 2023 01:24
@github-actions
Copy link

Test Results

   451 files  ±0     451 suites  ±0   2h 44m 54s ⏱️ - 5m 43s
   846 tests ±0     829 ✔️ ±0  17 💤 ±0  0 ±0 
2 804 runs  ±0  2 734 ✔️ ±0  70 💤 ±0  0 ±0 

Results for commit c68ae74. ± Comparison against base commit cdeb11d.

This pull request removes 530 and adds 530 tests. Note that renamed tests count towards both.
com.ibm.wala.cast.java.test.ECJIssue666Test ‑ testPeekErrorCase
com.ibm.wala.cast.java.test.ECJIssue667Test ‑ testDominanceFrontierCase
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testBinaryLiterals
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testCatchMultipleExceptionTypes
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testStringsInSwitch
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testTryWithResourcesStatement
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testTypeInferenceforGenericInstanceCreation
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testUnderscoresInNumericLiterals
com.ibm.wala.cast.java.test.ECJJavaIRTest ‑ testAnonymousClass
com.ibm.wala.cast.java.test.ECJJavaIRTest ‑ testArray1
…
com.ibm.wala.cast.java.test.ECJIssue666Test ‑ testPeekErrorCase()
com.ibm.wala.cast.java.test.ECJIssue667Test ‑ testDominanceFrontierCase()
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testBinaryLiterals()
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testCatchMultipleExceptionTypes()
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testStringsInSwitch()
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testTryWithResourcesStatement()
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testTypeInferenceforGenericInstanceCreation()
com.ibm.wala.cast.java.test.ECJJava17IRTest ‑ testUnderscoresInNumericLiterals()
com.ibm.wala.cast.java.test.ECJJavaIRTest ‑ testAnonymousClass()
com.ibm.wala.cast.java.test.ECJJavaIRTest ‑ testArray1()
…
This pull request removes 15 skipped tests and adds 15 skipped tests. Note that renamed tests count towards both.
com.ibm.wala.cast.js.rhino.callgraph.fieldbased.test.TestFieldBasedCG ‑ testBug2979
com.ibm.wala.cast.js.test.TestCorrelatedPairExtractionRhino ‑ test20
com.ibm.wala.cast.js.test.TestCorrelatedPairExtractionRhino ‑ test3
com.ibm.wala.cast.js.test.TestCorrelatedPairExtractionRhino ‑ test7
com.ibm.wala.cast.js.test.TestCorrelatedPairExtractionRhino ‑ test9
com.ibm.wala.cast.js.test.TestForInBodyExtractionRhino ‑ test14
com.ibm.wala.cast.js.test.TestForInBodyExtractionRhino ‑ test7
com.ibm.wala.cast.js.test.TestForInLoopHackRhino ‑ testJQueryWithHack
com.ibm.wala.cast.js.test.TestJQueryExamplesRhino ‑ testEx1
com.ibm.wala.cast.js.test.TestMediawikiCallGraphShapeRhino ‑ testSwineFlu
…
com.ibm.wala.cast.js.rhino.callgraph.fieldbased.test.TestFieldBasedCG ‑ testBug2979()
com.ibm.wala.cast.js.test.TestCorrelatedPairExtractionRhino ‑ test20()
com.ibm.wala.cast.js.test.TestCorrelatedPairExtractionRhino ‑ test3()
com.ibm.wala.cast.js.test.TestCorrelatedPairExtractionRhino ‑ test7()
com.ibm.wala.cast.js.test.TestCorrelatedPairExtractionRhino ‑ test9()
com.ibm.wala.cast.js.test.TestForInBodyExtractionRhino ‑ test14()
com.ibm.wala.cast.js.test.TestForInBodyExtractionRhino ‑ test7()
com.ibm.wala.cast.js.test.TestForInLoopHackRhino ‑ testJQueryWithHack()
com.ibm.wala.cast.js.test.TestJQueryExamplesRhino ‑ testEx1()
com.ibm.wala.cast.js.test.TestMediawikiCallGraphShapeRhino ‑ testSwineFlu()
…

@liblit liblit enabled auto-merge (rebase) July 26, 2023 01:32
@liblit
Copy link
Contributor Author

liblit commented Jul 26, 2023

Cool, we have the test summary that #1297 added! And it's doing exactly what I wanted, which is to show that this pull request causes no net change in the number of tests being run.

Granted, the report does think that 530 tests were removed and 530 new tests were added. That's because JUnit 4 and JUnit 5 name tests slightly differently: testPeekErrorCase under JUnit 4 becomes testPeekErrorCase() under JUnit 5 when the latter adds trailing parentheses. Ah well, I can live with that. The matching remove/add counts are strong evidence that we didn't lose any test cases during this bulk conversion.

Copy link
Member

@msridhar msridhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously didn't review every line of this one, but did some spot checking and it LGTM!

@liblit liblit merged commit 7b8d005 into wala:master Jul 26, 2023
8 checks passed
@liblit liblit deleted the junit-5/migrate-many-tests branch July 26, 2023 01:46
@liblit
Copy link
Contributor Author

liblit commented Jul 26, 2023

Thanks, @msridhar. This PR was for the big bulk of migrations that were mostly mechanized. I separated these migrations out specifically so that you could just skim, which is exactly what you did. Several other migrations will follow, all vastly smaller, for tests that required manual conversion. Those are more worth your careful attention than this bulk batch. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants