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.

Sign up for GitHub

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

Jump to bottom

Upgrade to gradle 8.4, junit 5. Fix configuration cache issues. #4069

Closed
wants to merge 5 commits into from
Closed

Upgrade to gradle 8.4, junit 5. Fix configuration cache issues. #4069

wants to merge 5 commits into from

Conversation

wwadge
Copy link
Contributor

@wwadge wwadge commented Jul 7, 2023 •
edited
Loading

This is a large MR, but mostly touches unit tests.

Fixes #4062 #3132 #4070

@wwadge wwadge mentioned this pull request Jul 7, 2023
Open
@wwadge wwadge force-pushed the master branch 8 times, most recently from 4923003 to e1376e9 Compare July 7, 2023 13:07
@wwadge wwadge force-pushed the master branch 8 times, most recently from be39b71 to 77f7f3d Compare July 10, 2023 07:30
@wwadge
Copy link
Contributor Author

wwadge commented Jul 11, 2023

Note: removal of "public" in all test methods is due to junit5 upgrade (triggered 1500 sonar code smell warnings...)

@wwadge wwadge force-pushed the master branch 5 times, most recently from b78c65f to 1a14b0d Compare July 11, 2023 21:00
@wwadge
Copy link
Contributor Author

wwadge commented Jul 20, 2023

"kokoro-windows" check failed, but I don't have access to view the check's output.

@wwadge wwadge force-pushed the master branch 2 times, most recently from f6c4727 to 2229911 Compare September 5, 2023 08:41
@wwadge wwadge changed the title Upgrade to gradle 8.2, junit 5. Fix configuration cache issues. Upgrade to gradle 8.3, junit 5. Fix configuration cache issues. Sep 5, 2023
@wwadge
Copy link
Contributor Author

wwadge commented Sep 11, 2023

Updated by removing the workaround for gradle which only applies to v6 (this PR bumps to v8.3). PR still fails on kokoro-windows but I'm afraid I don't have access to logs so I'm going to need your help to diagnose what's up with that.

@sonarcloud
Copy link

sonarcloud bot commented Sep 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

83.8% 83.8% Coverage
0.3% 0.3% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@sonarcloud
Copy link

sonarcloud bot commented Oct 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

84.7% 84.7% Coverage
0.3% 0.3% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@wwadge wwadge changed the title Upgrade to gradle 8.3, junit 5. Fix configuration cache issues. Upgrade to gradle 8.4, junit 5. Fix configuration cache issues. Oct 18, 2023
@loosebazooka
Copy link
Member

The windows issues are related to windows fs things I believe:

Caused by: java.io.IOException: Unable to delete directory 'C:\tmpfs\src\github\jib\jib-build-plan\build'
  Failed to delete some children. This might happen because a process has files open or has its working directory set in the target directory.
  - C:\tmpfs\src\github\jib\jib-build-plan\build\tmp\.cache\expanded\expanded.lock
  - C:\tmpfs\src\github\jib\jib-build-plan\build\tmp\.cache\expanded
  - C:\tmpfs\src\github\jib\jib-build-plan\build\tmp\.cache
  - C:\tmpfs\src\github\jib\jib-build-plan\build\tmp

@loosebazooka
Copy link
Member

This PR is almost too large to review. Can you find a way to separate the parts and we can look into getting it in? Perhaps separates out the gradle update, the plugin updates and Junit5 updates per project?

@wwadge
Copy link
Contributor Author

wwadge commented Oct 25, 2023

Hmmm probably a little difficult though I'd agree the PR is pretty big, but most of it is silly stuff like removing "public" from test methods (junit 5 thing, triggered a lot of code smells in Sonar). If you think it's worth the effort I'll try to give it a shot

@loosebazooka
Copy link
Member

Alright lemme give this review a shot and see how it goes.

loosebazooka
loosebazooka reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@loosebazooka loosebazooka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

so I started going through this, and it looks like there's still a lot of Junit4 (rules, assert, etc) -- I did not comment on all of them. I still think it makes sense to decouple the junit5 updates from the other changes related to cache configuration here.

build.gradle
Comment on lines +154 to +156
// /* GOOGLE JAVA FORMAT */
//
// /* CHECKSTYLE */
Copy link
Member

Choose a reason for hiding this comment

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

There's a few of these comment // things sprinkled around in this file, probably needs a cleanup

jib-build-plan/src/test/java/com/google/cloud/tools/jib/api/buildplan/AbsoluteUnixPathTest.java
Comment on lines 23 to 24
import org.junit.Assert;
import org.junit.Assume;
Copy link
Member

Choose a reason for hiding this comment

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

The move to junit5 is somewhat incomplete here?
org.junit.Assert -> org.junit.jupiter.api.Assertions
org.junit.Assume -> org.junit.jupiter.api.Assumptions

Copy link
Member

Choose a reason for hiding this comment

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

I think this might be true for across all test files?

jib-cli/src/integration-test/java/com/google/cloud/tools/jib/cli/JarCommandTest.java
import picocli.CommandLine;

public class JarCommandTest {
class JarCommandTest {

@ClassRule
Copy link
Member

Choose a reason for hiding this comment

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

@ClassRule -> @ExtendWith

@mpeddada1
Copy link
Contributor

+1 to @loosebazooka. @wwadge would it be possible to split this up into smaller PRs? It looks like we are trying to solve multiple problems here but it may help to break it down problem by problem.

@wwadge
Copy link
Contributor Author

wwadge commented Nov 6, 2023

@mpeddada1 yes I'll try to do that

This was referenced Nov 10, 2023
Closed
Open
@wwadge
Copy link
Contributor Author

wwadge commented Nov 10, 2023

Closing in favour of: #4139

@wwadge wwadge closed this Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loosebazooka loosebazooka loosebazooka left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Jib Gradle Plugin emits warning on gradle 8.2 due to referencing deprecated convention.
5 participants
@wwadge @mpeddada1 @loosebazooka @kokoro-team @alicejli