-
Notifications
You must be signed in to change notification settings - Fork 39
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
Make Checkstyle integration test macOS-compatible #867
Conversation
/integration-test |
Looks good. No mutations were possible for these changes. |
/integration-test |
1 similar comment
/integration-test |
Looks good. No mutations were possible for these changes. |
/integration-test |
Looks good. No mutations were possible for these changes. |
/integration-test |
1 similar comment
/integration-test |
5975326
to
0c8c90d
Compare
Looks good. No mutations were possible for these changes. |
0c8c90d
to
380fcb4
Compare
Looks good. No mutations were possible for these changes. |
1 similar comment
Looks good. No mutations were possible for these changes. |
Either @benhalasi or @mohamedsamehsalah can you try to run this on your Mac and let us know if it works? |
Looks good. No mutations were possible for these changes. |
Ah, I mean the |
I've run it:
|
a95e69b
to
1ccf32d
Compare
Thanks @benhalasi , I added something I didn't move over. Could you try again? |
Looks good. No mutations were possible for these changes. |
1 similar comment
Looks good. No mutations were possible for these changes. |
Yep, it runs now, but it says there are unexpected changes:
|
Ah those were the changes that I introduced, they shouldn't be there. Let me fix that. Thanks for checking @benhalasi 🚀 ! |
1ccf32d
to
0e9b183
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased and added a commit; further review TBD.
diff --git a/pom.xml b/pom.xml | ||
index ca1f3d5..33ad480 100644 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below: let's omit this meta-data.
6b3d076
to
4638392
Compare
2c1f289
to
8cdc569
Compare
Rebased and resolved conflict. |
Looks good. No mutations were possible for these changes. |
/integration-test |
@benhalasi or @mohamedsamehsalah can one of you take a look and try to run the |
Looks good. No mutations were possible for these changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
run on MacOS, openjdk 17.0.3 2022-04-19
mvnd -DskipTests -Dverification.skip clean install
./integration-tests/checkstyle.sh
git status
didn't produce any diff!
fa13fd4
to
0d9ef72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased the branch and added two commits.
Suggested commit message:
Make Checkstyle integration test macOS-compatible (#867)
NB: Initially I observed the following error:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.12.1:compile (error-prone-compile) on project checkstyle: Fatal error compiling: DeleteEmptyMethod is not a valid checker name -> [Help 1]
This happened because the workshop/target
directory of the workshop
Git branch still existed. This is a bug in the script we may at some point want to fix. (But it's not related to the changes in this PR.)
integration-tests/checkstyle.sh
Outdated
| sed -r 's,(.*),-Xep:\1:WARN,' \ | ||
| grep -v ErrorProneRuntimeClasspath \ | ||
| paste -s -d ' ' | ||
| xargs -0 ${grep_command} -hoP '[^.]+$' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| xargs -0 ${grep_command} -hoP '[^.]+$' \ | |
| xargs -0 "${grep_command}" -hoP '[^.]+$' \ |
integration-tests/checkstyle.sh
Outdated
echo "Execute the following command to see the changes:" | ||
echo "$ diff -u ${expected_changes} ${actual_changes}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below: the script already prints this output as part of the if
-statement; we should call that out.
But also: actual_changes
refers to a file in a temporary directory that is cleaned up when the script exits. I'll propose something.
/integration-test |
Looks good. No mutations were possible for these changes. |
This build was skipped. Not sure why 🤔. |
/integration-test |
0d9ef72
to
c178c0b
Compare
It ran successfully when I posted a comment 🤔. So I'd say let's merge, right? Rebased. |
Looks good. No mutations were possible for these changes. |
Quality Gate passedIssues Measures |
Sure, then let's merge 👍. (I would also have expected it to show up in the check overview, but alas.) |
No description provided.