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

WIP: Add on-demand integration test workflow #836

Closed
wants to merge 12 commits into from
58 changes: 58 additions & 0 deletions .github/workflows/integration-test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
name: On-demand integration test
on:
# run action every time a new comment is created
Copy link
Member

Choose a reason for hiding this comment

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

We can omit this :).

issue_comment:
types: [ created ]
# enable this, to test the action without changing default branch
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# enable this, to test the action without changing default branch
# XXX: Enable this, to test the action without changing default branch.

push:
branches: [ gdejong/integration-test-action ]

permissions:
contents: read
jobs:
on_demand_integration_test:
name: On-demand integration test
# Only run the integration test if the created comment is on a pull request
# and contains the string `/integration-test`
# disable this to avoid having to change the default branch on EPS
# if: |
# github.event.issue.pull_request &&
# contains(github.event.comment.body, '/integration-test')
# XXX TODO: Device matrix?
Copy link
Member

Choose a reason for hiding this comment

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

Also not for now I'd say, let's omit.

runs-on: ubuntu-latest
steps:
- name: Check out code
uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 # v4.1
with:
submodules: 'recursive'
# ensure we fetch ALL commits and tags!
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# ensure we fetch ALL commits and tags!
# Ensure that all commits and tags are being fetched.

fetch-depth: '0'
persist-credentials: false
- name: Set git config
run: |
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

git config --global user.email "cody@picnic.tech"
git config --global user.name "cody"
- name: Set up JDK
uses: actions/setup-java@0ab4596768b603586c0de567f2430c30f5b0d2b0 # v3.13.0
with:
# XXX TODO: java version matrix?
Copy link
Member

Choose a reason for hiding this comment

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

Lets omit this for now, maybe add an XXX on the top of the file as for now.

java-version: 17.0.8
distribution: temurin
cache: maven
- name: Display build environment details
run: mvn --version
- name: Display grep version
run: grep --version
- name: Install error-prone-support snapshot
run: mvn -T1C install -DskipTests -Dverification.skip
- name: Run integration test
# ensure this script is executed in the integration-tests directory
working-directory: ./integration-tests
run: bash ./run.sh
- name: Upload diff artifacts
uses: actions/upload-artifact@a8a3f3ad30e3422c9c7b888a15615d19a852ae32 # v3.1.3
with:
name: integration-test-diffs
path: |
./integration-tests/checkstyle-checkstyle-10.9.3-expected-changes.patch
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 we can use * instead of the specific versions 😄.
See some examples here: https://github.com/actions/upload-artifact.

./integration-tests/checkstyle-checkstyle-10.9.3-expected-warnings.txt
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "integration-tests/checkstyle"]
path = integration-tests/checkstyle
url = git@github.com:checkstyle/checkstyle.git
1 change: 1 addition & 0 deletions integration-tests/checkstyle
Submodule checkstyle added at 7b8d36
54,939 changes: 54,939 additions & 0 deletions integration-tests/checkstyle-checkstyle-10.9.3-expected-changes.patch

Large diffs are not rendered by default.

Large diffs are not rendered by default.

131 changes: 131 additions & 0 deletions integration-tests/checkstyle-checkstyle-10.9.3-init.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
diff --git a/config/archunit-store/slices-should-be-free-of-cycles-suppressions b/config/archunit-store/slices-should-be-free-of-cycles-suppressions
index ea99a44ee..05a77e1e6 100644
--- a/config/archunit-store/slices-should-be-free-of-cycles-suppressions
+++ b/config/archunit-store/slices-should-be-free-of-cycles-suppressions
@@ -1351,8 +1351,8 @@ Cycle detected: Slice checks.javadoc -> \
- Constructor <com.puppycrawl.tools.checkstyle.checks.javadoc.AtclauseOrderCheck.<init>()> calls method <com.puppycrawl.tools.checkstyle.utils.TokenUtil.asBitSet([I)> in (AtclauseOrderCheck.java:177)\
- Method <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTagContinuationIndentationCheck.visitJavadocToken(com.puppycrawl.tools.checkstyle.api.DetailNode)> calls method <com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getNextSibling(com.puppycrawl.tools.checkstyle.api.DetailNode)> in (JavadocTagContinuationIndentationCheck.java:177)\
- Method <com.puppycrawl.tools.checkstyle.checks.javadoc.RequireEmptyLineBeforeBlockTagGroupCheck.isAnotherTagBefore(com.puppycrawl.tools.checkstyle.api.DetailNode)> calls method <com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getPreviousSibling(com.puppycrawl.tools.checkstyle.api.DetailNode)> in (RequireEmptyLineBeforeBlockTagGroupCheck.java:177)\
+ - Method <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocNodeImpl.toString()> calls method <com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getTokenName(int)> in (JavadocNodeImpl.java:178)\
- Method <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTagInfo$8.isValidOn(com.puppycrawl.tools.checkstyle.api.DetailAST)> calls method <com.puppycrawl.tools.checkstyle.utils.ScopeUtil.isLocalVariableDef(com.puppycrawl.tools.checkstyle.api.DetailAST)> in (JavadocTagInfo.java:178)\
- - Method <com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocNodeImpl.toString()> calls method <com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getTokenName(int)> in (JavadocNodeImpl.java:179)\
- Method <com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck.setJavadocTokens([Ljava.lang.String;)> calls method <com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getTokenId(java.lang.String)> in (AbstractJavadocCheck.java:181)\
- Method <com.puppycrawl.tools.checkstyle.checks.javadoc.RequireEmptyLineBeforeBlockTagGroupCheck.isAnotherTagBefore(com.puppycrawl.tools.checkstyle.api.DetailNode)> calls method <com.puppycrawl.tools.checkstyle.utils.JavadocUtil.getPreviousSibling(com.puppycrawl.tools.checkstyle.api.DetailNode)> in (RequireEmptyLineBeforeBlockTagGroupCheck.java:183)\
- Method <com.puppycrawl.tools.checkstyle.checks.javadoc.NonEmptyAtclauseDescriptionCheck.hasOnlyEmptyText(com.puppycrawl.tools.checkstyle.api.DetailNode)> calls method <com.puppycrawl.tools.checkstyle.utils.CommonUtil.isBlank(java.lang.String)> in (NonEmptyAtclauseDescriptionCheck.java:185)\
diff --git a/pom.xml b/pom.xml
index 6169c87e0..491c7fa88 100644
--- a/pom.xml
+++ b/pom.xml
@@ -230,10 +230,12 @@
<pitest.junit5.plugin.version>1.1.2</pitest.junit5.plugin.version>
<pitest.accelerator.junit5.plugin.version>1.0.4</pitest.accelerator.junit5.plugin.version>
<sonar.test.exclusions>**/test/resources/**/*,**/it/resources/**/*</sonar.test.exclusions>
+ <assertj.version>3.24.2</assertj.version>
<junit.version>5.9.2</junit.version>
<forbiddenapis.version>3.4</forbiddenapis.version>
<json-schema-validator.version>1.2.0</json-schema-validator.version>
- <error-prone.version>2.18.0</error-prone.version>
+ <error-prone.version>2.22.0</error-prone.version>
+ <error-prone-support.version>0.14.0</error-prone-support.version>
<checkerframework.version>3.27.0</checkerframework.version>
</properties>

@@ -277,6 +279,12 @@
</dependency>

<!-- test scope stuff -->
+ <dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
+ <version>${assertj.version}</version>
+ <scope>test</scope>
+ </dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
@@ -2305,7 +2313,7 @@
<arg>-Xpkginfo:always</arg>
<arg>-XDcompilePolicy=simple</arg>
<arg>
- -Xplugin:ErrorProne
+ -Xplugin:ErrorProne ${error-prone.flags}
</arg>
</compilerArgs>
<annotationProcessorPaths>
@@ -2314,6 +2322,16 @@
<artifactId>error_prone_core</artifactId>
<version>${error-prone.version}</version>
</path>
+ <path>
+ <groupId>tech.picnic.error-prone-support</groupId>
+ <artifactId>error-prone-contrib</artifactId>
+ <version>${error-prone-support.version}</version>
+ </path>
+ <path>
+ <groupId>tech.picnic.error-prone-support</groupId>
+ <artifactId>refaster-runner</artifactId>
+ <version>${error-prone-support.version}</version>
+ </path>
</annotationProcessorPaths>
</configuration>
</execution>
@@ -2358,7 +2376,8 @@
<arg>-XDcompilePolicy=simple</arg>
<arg>
-Xplugin:ErrorProne \
- -XepExcludedPaths:.*[\\/]resources[\\/].*
+ -XepExcludedPaths:.*[\\/]resources[\\/].* \
+ ${error-prone.flags}
</arg>
</compilerArgs>
<annotationProcessorPaths>
@@ -2367,6 +2386,16 @@
<artifactId>error_prone_core</artifactId>
<version>${error-prone.version}</version>
</path>
+ <path>
+ <groupId>tech.picnic.error-prone-support</groupId>
+ <artifactId>error-prone-contrib</artifactId>
+ <version>${error-prone-support.version}</version>
+ </path>
+ <path>
+ <groupId>tech.picnic.error-prone-support</groupId>
+ <artifactId>refaster-runner</artifactId>
+ <version>${error-prone-support.version}</version>
+ </path>
</annotationProcessorPaths>
</configuration>
</execution>
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinter.java b/src/main/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinter.java
index 61c59b613..9019964ad 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinter.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/DetailNodeTreeStringPrinter.java
@@ -63,6 +63,7 @@ public final class DetailNodeTreeStringPrinter {
* @return DetailNode tree
* @throws IllegalArgumentException if there is an error parsing the Javadoc.
*/
+ @SuppressWarnings("CheckArgumentWithMessage")
public static DetailNode parseJavadocAsDetailNode(DetailAST blockComment) {
final JavadocDetailNodeParser parser = new JavadocDetailNodeParser();
final ParseStatus status = parser.parseJavadocAsDetailNode(blockComment);
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocNodeImpl.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocNodeImpl.java
index fd90c4259..0c1b4f141 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocNodeImpl.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocNodeImpl.java
@@ -20,7 +20,6 @@
package com.puppycrawl.tools.checkstyle.checks.javadoc;

import java.util.Arrays;
-import java.util.Objects;
import java.util.Optional;

import com.puppycrawl.tools.checkstyle.api.DetailNode;
@@ -180,7 +179,7 @@ public class JavadocNodeImpl implements DetailNode {
+ ", text='" + text + '\''
+ ", lineNumber=" + lineNumber
+ ", columnNumber=" + columnNumber
- + ", children=" + Objects.hashCode(children)
+ + ", children=" + Arrays.hashCode(children)
+ ", parent=" + parent + ']';
}

111 changes: 111 additions & 0 deletions integration-tests/run.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
#!/usr/bin/env bash

set -e -u -o pipefail

project=checkstyle
revision=checkstyle-10.9.3

if [ "${#}" -gt 1 ] || [[ ${1:---sync} != '--sync' ]]; then
echo "Usage: ${0} [--sync]"
exit 1
fi
do_sync="${1:-}"

error_prone_support_version="$(
mvn -f .. help:evaluate -Dexpression=project.version -q -DforceStdout
)"

error_prone_shared_flags='-XepExcludedPaths:(\Q${project.basedir}${file.separator}src${file.separator}\E(it|test)\Q${file.separator}resources\E|\Q${project.build.directory}${file.separator}\E).*'

error_prone_patch_flags="${error_prone_shared_flags} -XepPatchLocation:IN_PLACE -XepPatchChecks:$(
find .. -path "*/META-INF/services/com.google.errorprone.bugpatterns.BugChecker" \
| xargs grep -hoP '[^.]+$' \
| paste -s -d ','
)"

error_prone_validation_flags="${error_prone_shared_flags} -XepDisableAllChecks $(
find .. -path "*/META-INF/services/com.google.errorprone.bugpatterns.BugChecker" \
| xargs grep -hoP '[^.]+$' \
| sed -r 's,(.*),-Xep:\1:WARN,' \
| paste -s -d ' '
)"

validation_log_file="$(mktemp)"
trap 'rm -rf -- "${validation_log_file}"' INT TERM HUP EXIT

echo "Error Prone Support version: ${error_prone_support_version}"
echo "Error Prone patch flags: ${error_prone_patch_flags}"
echo "Error Prone validation flags: ${error_prone_validation_flags}"

pushd "${project}"

git checkout -f "${revision}"
git apply < "../${project}-${revision}-init.patch"
git commit -m 'dependency: Introduce Error Prone Support' .

mvn com.spotify.fmt:fmt-maven-plugin:2.19:format \
-DadditionalSourceDirectories='${project.basedir}${file.separator}src${file.separator}it${file.separator}java'
git commit -m 'minor: Reformat using Google Java Format' .

function apply_patch() {
local current_diff="${1}"

mvn clean package com.spotify.fmt:fmt-maven-plugin:2.19:format \
-DadditionalSourceDirectories='${project.basedir}${file.separator}src${file.separator}it${file.separator}java' \
-Perror-prone-compile,error-prone-test-compile \
-Derror-prone.flags="${error_prone_patch_flags}" \
-Derror-prone-support.version="${error_prone_support_version}" \
-DskipTests

local new_diff="$(git diff | shasum --algorithm 256)"

if [ "${current_diff}" != "${new_diff}" ]; then
apply_patch "${new_diff}"
fi
}

apply_patch "$(git diff | shasum --algorithm 256)"

# disable sync mechanism, we just want to upload the changes
baseline_patch="../${project}-${revision}-expected-changes.patch"
# if [ -n "${do_sync}" ]; then
echo 'Saving changes...'
git diff > "${baseline_patch}"
# else
# echo 'Inspecting changes...'
# if ! diff -u "${baseline_patch}" <(git diff); then
# echo 'There are unexpected changes.'
# exit 1
# fi
# fi

# Validate the results.
#
# - The `metadataFilesGenerationAllFiles` test is skipped because is makes line
# number assertions that will fail when the code is formatted or patched.
# - The `allCheckSectionJavaDocs` test is skipped because is validates that
# Javadoc has certain closing tags that are removed by Google Java Format.
# XXX: Figure out why the `validateCliDocSections` test fails.
echo "Validation file: ${validation_log_file}"
mvn clean package \
-Perror-prone-compile,error-prone-test-compile \
-Derror-prone.flags="${error_prone_validation_flags}" \
-Derror-prone-support.version="${error_prone_support_version}" \
-Dmaven.compiler.showWarnings \
| tee "${validation_log_file}"
echo "Finished validation run!"

baseline_warnings="../${project}-${revision}-expected-warnings.txt"
# note: added '*' in the final grep, required in order to get matches in GNU grep 3.11
# disable sync mechanism, we just want to upload the expected warnings
generated_warnings="$(grep -oP "(?<=^\\Q[WARNING] ${PWD}/\\E).*" "${validation_log_file}" | grep -P '\]*\[')"
# if [ -n "${do_sync}" ]; then
echo 'Saving emitted warnings...'
echo "${generated_warnings}" > "${baseline_warnings}"
# else
# echo 'Inspecting emitted warnings...'
# if ! diff -u "${baseline_warnings}" <(echo "${generated_warnings}"); then
# echo 'Diagnostics output changed.'
# exit 1
# fi
# fi