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

Upgrade Checkstyle integration test target 10.14.0 -> 10.21.1 #1425

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

rickie
Copy link
Member

@rickie rickie commented Nov 18, 2024

Suggested commit message (similar to the previous one):

Sync Checkstyle integration test (#1425)
    
Summary of changes:
- Update `checkstyle-init.patch`. 
- Update `checkstyle-expected-changes.patch`.
- Update `checkstyle-expected-warnings.txt`
- Test against version 10.21.1 rather than 10.14.0.

Or my slightly preferred alternative:

Sync Checkstyle integration test (#1425)
    
By updating and testing against 10.21.1. 

@rickie rickie requested a review from Stephan202 November 18, 2024 08:08
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@@ -366,6 +366,12 @@
<version>1.4.1</version>
@@ -368,6 +368,12 @@
<version>1.4.4</version>
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, this one is unexpected, if you don't recognize this one, I'll try to remove it @Stephan202 😄

Copy link
Member

Choose a reason for hiding this comment

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

This is a change in the patch context, not a change in the changes made by the patch. So this is not unexpected 👍

@rickie
Copy link
Member Author

rickie commented Nov 18, 2024

/integration-test

@rickie rickie added the chore A task not related to code (build, formatting, process, ...) label Nov 18, 2024
@rickie rickie added this to the 0.20.0 milestone Nov 18, 2024
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Suggested commit message:

Upgrade Checkstyle integration test target 10.14.0 -> 10.21.1 (#1425)

(This phrasing is a bit different from what we did before, but if we start to file this kind of PR more often, then the summary messages will be more informative.)

@@ -366,6 +366,12 @@
<version>1.4.1</version>
@@ -368,6 +368,12 @@
<version>1.4.4</version>
Copy link
Member

Choose a reason for hiding this comment

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

This is a change in the patch context, not a change in the changes made by the patch. So this is not unexpected 👍

@Stephan202 Stephan202 changed the title Sync Checkstyle integration test Upgrade Checkstyle integration test target 10.14.0 -> 10.21.1 Nov 18, 2024
@rickie
Copy link
Member Author

rickie commented Nov 18, 2024

Instead of target we could say tag? 🤔

@Stephan202
Copy link
Member

Well, the integration test targets a specific Checkstyle version. The specified tag is the test target.

Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah left a comment

Choose a reason for hiding this comment

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

Thanks for the summary, LGTM 🚀

I also prefer the alternative message 😄

Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah left a comment

Choose a reason for hiding this comment

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

I meant to approve ✅

@Stephan202 Stephan202 merged commit 27e9fe7 into master Nov 18, 2024
16 checks passed
@Stephan202 Stephan202 deleted the rossendrijver/update_checkstyle branch November 18, 2024 17:00
@rickie
Copy link
Member Author

rickie commented Nov 18, 2024

@Stephan202 Two questions;

  • As we are going to add quite a few PRs related to the integration-tests, I think it'd be a good idea to add a separate label for it.
  • The Checkstyle test shows some JDK related error all the way at the end of the test. What do you think about adding a simple check in checkstyle.sh that validates you're not using an JDK higher than 17? As that doesn't give errors for me.

@Stephan202
Copy link
Member

Stephan202 commented Nov 18, 2024

Re 1: not opposed, but as a non-maintainer reading the release notes, I'm not sure I'd care much about the details of our testing setup. We could of course introduce a separate label and then map it to the same release notes section. If you think that helps with filtering: works for me.

Re 2: we'd have to check whether this is compatible with GitHub actions, but perhaps it'd be even nicer if stuff "just worked", like this. The alternative is to parse mvn -version output and compare version numbers (possibly also for Maven): possible but not trivial. (But perhaps sort -h works well enough, didn't test.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore A task not related to code (build, formatting, process, ...)
Development

Successfully merging this pull request may close these issues.

3 participants