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

Allow backwards compatibility with JUnit 4 @Rules in JUnit 5 #526

Merged
merged 2 commits into from Mar 23, 2020

Conversation

@CydeWeys
Copy link
Member

CydeWeys commented Mar 20, 2020

This allows us to defer having to re-implement all of our JUnit 4 Rules as JUnit
5 extensions for now, while continuing to in-place upgrade all existing JUnit 4
test classes to JUnit 5.

As proof of concept, this upgrades PremiumListUtils (which uses AppEngineRule,
our largest and most complicated @rule) to use the JUnit 5 test runner.


This change is Reviewable

This allows us to defer having to re-implement all of our JUnit 4 Rules as JUnit
5 extensions for now, while continuing to in-place upgrade all existing JUnit 4
test classes to JUnit 5.

As proof of concept, this upgrades PremiumListUtils (which uses AppEngineRule,
our largest and most complicated @rule) to use the JUnit 5 test runner.
@CydeWeys CydeWeys requested a review from weiminyu Mar 20, 2020
@googlebot googlebot added the cla: yes label Mar 20, 2020
Copy link
Collaborator

weiminyu left a comment

Reviewed 9 of 55 files at r1.
Reviewable status: 9 of 55 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu)


package-lock.json, line 10 at r1 (raw file):

      "version": "1.3.7",
      "resolved": "https://registry.npmjs.org/accepts/-/accepts-1.3.7.tgz",
      "integrity": "sha512-Il80Qs2WjYlJIBNzNkK6KYqlVMTbZLXgHx2oT0pU/fjRHyEp+PEfEPY0R3WCwAGVOtauxh1hOxNgIf5bv7dQpA==",

Revert this file since this file is not relevant to the purpose of this PR?


core/src/main/java/google/registry/module/backend/BackendRequestComponent.java, line 144 at r1 (raw file):

  RefreshDnsAction refreshDnsAction();
  RefreshDnsOnHostRenameAction refreshDnsOnHostRenameAction();

Has anything changed beside the two blank lines?
Did the javaIncremantalFormatApply command force this change?

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 9 of 55 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu)


core/src/main/java/google/registry/module/backend/BackendRequestComponent.java, line 144 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

Has anything changed beside the two blank lines?
Did the javaIncremantalFormatApply command force this change?

Yes, the formatter forced this change.

Unfortunately that's a pretty safe guess when you see issues like this.

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 9 of 55 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu)


core/src/main/java/google/registry/module/backend/BackendRequestComponent.java, line 144 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Yes, the formatter forced this change.

Unfortunately that's a pretty safe guess when you see issues like this.

I applied the formatter to this whole file so that the changes aren't so jarring. I don't know why it complained about this file though; all I can think of is that this RelockDomainAction line was recently added (but not by me). Seems like something is wrong with incremental formatting.

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 9 of 55 files reviewed, 2 unresolved discussions (waiting on @weiminyu)


package-lock.json, line 10 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

Revert this file since this file is not relevant to the purpose of this PR?

These changes happened because I ran update-dependency to add the new JUnit library. If I revert these changes, won't they just be picked up the next time someone else runs update-dependency anyway?

Copy link
Collaborator

weiminyu left a comment

Reviewable status: 9 of 55 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu)


package-lock.json, line 10 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

These changes happened because I ran update-dependency to add the new JUnit library. If I revert these changes, won't they just be picked up the next time someone else runs update-dependency anyway?

This file can be unstable for extended period of time when every build would cause change. Changes are caused by running npm install, and are not related to java changes. Not a big deal though.

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 9 of 55 files reviewed, 2 unresolved discussions (waiting on @weiminyu)


package-lock.json, line 10 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

This file can be unstable for extended period of time when every build would cause change. Changes are caused by running npm install, and are not related to java changes. Not a big deal though.

OK, submitting as-is just to not have to deal with backing out this file and re-running the presubmit. Were I feeling better I might have reverted this file instead, but it sounds like it doesn't much matter either way.

@CydeWeys CydeWeys merged commit fe760d7 into google:master Mar 23, 2020
6 of 7 checks passed
6 of 7 checks passed
code-review/reviewable 46 files, 2 discussions left (weiminyu)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details
@CydeWeys CydeWeys deleted the CydeWeys:moar-junit5 branch Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.