Skip to content

[CALCITE-2457] JUnit4 -> JUnit5#1621

Merged
vlsi merged 6 commits intoapache:masterfrom
vlsi:junit5
Dec 3, 2019
Merged

[CALCITE-2457] JUnit4 -> JUnit5#1621
vlsi merged 6 commits intoapache:masterfrom
vlsi:junit5

Conversation

@vlsi
Copy link
Contributor

@vlsi vlsi commented Dec 2, 2019

Checklist:

  • @Test
  • Code style (e.g. line length after migration)
  • Assert
  • @Parameterized
  • Assume

Out of scope: cassandra, mongodb, elasticsearch, geode, linq4j (see junit4=true in gradle.properties)

This removes JUnit4 from most of the modules, so there will be less confusion between @Test annotations.

@vlsi vlsi force-pushed the junit5 branch 9 times, most recently from d746b82 to d8fcf2c Compare December 3, 2019 17:10
@vlsi vlsi changed the title WIP: JUnit4 -> JUnit5 [CALCITE-2457] JUnit4 -> JUnit5 Dec 3, 2019
@vlsi vlsi marked this pull request as ready for review December 3, 2019 17:10
@vlsi vlsi added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Dec 3, 2019
@vlsi vlsi force-pushed the junit5 branch 2 times, most recently from 769a7e9 to 66afcf3 Compare December 3, 2019 17:58
targetExclude(*javaccGeneratedPatterns + "**/test/java/*.java")
licenseHeaderFile(licenseHeaderFile)
if (!project.props.bool("junit4", default = false)) {
replace("junit5: Test", "org.junit.Test", "org.junit.jupiter.api.Test")
Copy link
Member

Choose a reason for hiding this comment

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

Is string/replace still necessary if most of the tests are migrated already ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can drop that later.

The use case is as follows:

  1. You drop junit4=true from mongodb/gradle.properties, and execute ./gradlew spotlessApply. It would use the replacements to automatically replace the bits in mongodb module.

  2. When somebody implemented JUnit4-based test, then they rebase on top of master.
    The replacement rules would convert the code to the new format automatically!
    That is nice, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, assertEquals, assertTrue, and frields have different APIs between 4 and 5, so they can't be converted with a regexp :-(

@vlsi vlsi merged commit e89784c into apache:master Dec 3, 2019
@asereda-gs
Copy link
Member

@vlsi does it make sense to squash commits ? There is no much information in intermediate changes.

@vlsi
Copy link
Contributor Author

vlsi commented Dec 3, 2019

I think it helps to review the changes. For instance, mechanical edits like "alter package names" are not that interesting, and it is even hard to view in GitHub UI.

We have 3700+ commits, so 7 vs 1 won't make a difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants