Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

Bump testing libraries to their latest possible versions #449

Merged
merged 8 commits into from
Feb 1, 2023

Conversation

TWiStErRob
Copy link
Contributor

@TWiStErRob TWiStErRob commented Jan 13, 2023

See individual commits for details, copied here for convenience:


  • Dependency version bumps:
    • Bump commons-io from 2.4 to 2.11.0
      • Replace usage of deprecated readFileToString(File) with readFileToString(File, Charset)
    • Bump junit from 4.12 to 4.13
      • Replace usages of deprecated junit.Assert.assertThat with hamcrest.MatcherAssert.assertThat
    • Bump hamcrest-library from 1.3 to 2.2
    • Bump mockito-core from 2.11.0 to 4.11.0
      • Mockito 5.0 (released at the same weeks as I raised the PR) requires Java 11 bytecode, so it's not viable yet, because CI runs on [8,11].
    • Bump jacoco from 0.8.6 to 0.8.8
    • Bump checkstyle from 8.18 to 8.37
      • 8.38 requires fixing 12 MissingJavadocType failures on public main source set classes.
      • 9.3 can be used once 8.38 javadocs are fixed, because that's the latest version that supports Java 8.
      • Done up to here in Fix all known javadoc issues #452
      • 10.x requires Java 11 in Gradle runtime, while the .github/workflows/unit-tests.yaml is executed on Java 8, Checkstyle 10+ is not viable.
  • Have verifyGoogleFormat task run before checkstyle tasks (to suggest fixing via ./gradlew googleJavaFormat)

8.38 requires fixing 12 MissingJavadocType failures on public main source set classes.
9.3 can be used once 8.38 javadocs are fixed, because that's the latest version that supports Java 8.
10.x requires Java 11 in Gradle runtime, while the .github/workflows/unit-tests.yaml is executed on Java 8, Checkstyle 10+ is not viable.
Copy link
Contributor

@emmileaf emmileaf left a comment

Choose a reason for hiding this comment

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

@TWiStErRob Thanks for making these updates! Overall LGTM, apologies for the hassle that some conflicts will need to be resolved here since we've merged #448.

The individual commits were very helpful in tracking the different changes - I’ve updated the PR description with details according to your commit messages for better visibility, please edit or comment if there is anything I am missing or miscapturing here.

build.gradle.kts Outdated
Comment on lines 189 to 198
tasks.checkstyleMain.configure {
// Set up a soft dependency so that verifyGoogleFormat suggests running googleJavaFormat,
// before devs start fixing individual checkstyle violations manually.
shouldRunAfter(tasks.verifyGoogleJavaFormat)
}
tasks.named("checkstyleIntegTest").configure {
// Set up a soft dependency so that verifyGoogleFormat suggests running googleJavaFormat,
// before devs start fixing individual checkstyle violations manually.
shouldRunAfter(tasks.verifyGoogleJavaFormat)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if I'm understanding the intention here correctly, perhaps something like this could also work?

tasks.withType<Checkstyle> {
  shouldRunAfter(tasks.verifyGoogleJavaFormat)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: never ever ever ever use withType<> {} it's eager Gradle configuration (it'll create all tasks).

withType<>().configureEach {} is the lazy new API from docs:
image

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'm planning on opening a PR eventually to make this plugin lazy(er).

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: never ever ever ever use withType<> {} it's eager Gradle configuration (it'll create all tasks).

Ah, thanks for catching this and replacing with the alternative! TIL :)

@TWiStErRob
Copy link
Contributor Author

apologies for the hassle that some conflicts will need to be resolved here

No worries, I caused the conflicts, I made them auto-resolvable in IDEA. Done in 9c55a6a.

Copy link
Contributor

@emmileaf emmileaf left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@emmileaf emmileaf merged commit 33babd4 into GoogleCloudPlatform:master Feb 1, 2023
@TWiStErRob TWiStErRob deleted the tests branch February 1, 2023 14:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants