Introduce Checkstyle integration test #865
Conversation
|
Looks good. No mutations were possible for these changes. |
| # If requested by means of a pull request comment, runs integration tests | ||
| # against the project, using the code found on the pull request branch. | ||
| # XXX: Generalize this to a matrix build of multiple integration tests, | ||
| # possibly using multiple JDK or OS versions. | ||
| name: "Integration tests" | ||
| on: | ||
| issue_comment: | ||
| types: [ created ] | ||
| permissions: | ||
| contents: read | ||
| jobs: | ||
| run-integration-tests: | ||
| name: On-demand integration test | ||
| if: | | ||
| github.event.issue.pull_request && contains(github.event.comment.body, '/integration-test') | ||
| runs-on: ubuntu-22.04 | ||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
| with: | ||
| persist-credentials: false | ||
| - name: Set up JDK | ||
| uses: actions/setup-java@0ab4596768b603586c0de567f2430c30f5b0d2b0 # v3.13.0 | ||
| with: | ||
| java-version: 17.0.8 | ||
| distribution: temurin | ||
| cache: maven | ||
| - name: Install project to local Maven repository | ||
| run: mvn -T1C install -DskipTests -Dverification.skip | ||
| - name: Run integration test | ||
| run: ./integration-tests/checkstyle-10.12.4.sh |
There was a problem hiding this comment.
The contents in this file are an adaptation of work by @oxkitsune; tnx!
| + <dependency> | ||
| + <groupId>org.assertj</groupId> | ||
| + <artifactId>assertj-core</artifactId> | ||
| + <version>${assertj.version}</version> |
There was a problem hiding this comment.
The version is indeed always supplied externally. The reasoning behind this decision is that it is likely to minimize maintenance impact; ideally we don't need to frequently modify initial patch files such as this one.
| + // XXX: Don't reorder arguments to `picocli.CommandLine.Option#names`. | ||
| + @SuppressWarnings("LexicographicalAnnotationAttributeListing") |
There was a problem hiding this comment.
We should file a separate improvement PR for this.
| + // XXX: File PR to replace `.collect(toSet())` with `.collect(toCollection(HashSet::new))`. | ||
| + // XXX: Update `CollectorMutability` to recognize cases such as this one, where the created | ||
| + // collection is clearly modified. | ||
| + @SuppressWarnings("CollectorMutability") |
| + // XXX: Don't suggest `ImmutableSet.of(elem)` for nullable `elem`. | ||
| + @SuppressWarnings("ImmutableSetOf1") |
There was a problem hiding this comment.
I once started a branch to enable this sort of thing, but hit some roadblocks. To be reviewed.
| test_name="$(basename "${0}" .sh)" | ||
| project=checkstyle | ||
| [email protected]:checkstyle/checkstyle.git | ||
| revision=checkstyle-10.12.4 |
There was a problem hiding this comment.
Perhaps we can also convince Renovate to track new tags. TBD whether that's worth it.
| # Make sure that the targeted tag of the project's Git repository is checked | ||
| # out. | ||
| project_root="${repos_root}/${project}" | ||
| if [ ! -d "${project_root}" ]; then | ||
| # The repository has not yet been cloned; create a shallow clone. | ||
| git clone --branch "${revision}" --depth 1 "${repository}" "${project_root}" | ||
| else | ||
| # The repository does already appear to exist. Try to check out the requested | ||
| # tag if possible, and fetch it otherwise. | ||
| # | ||
| # Under certain circumstances this does not cause the relevant tag to be | ||
| # created, so if necessary we manually create it. | ||
| git -C "${project_root}" checkout --force "${revision}" 2>/dev/null \ | ||
| || ( | ||
| git -C "${project_root}" fetch --depth 1 "${repository}" "${revision}" \ | ||
| && git -C "${project_root}" checkout --force FETCH_HEAD \ | ||
| && git -C "${project_root}" tag "${revision}" || true | ||
| ) | ||
| fi |
There was a problem hiding this comment.
On GitHub Actions there'll always be a new checkout, but for local development it's very convenient if repeated script runs don't trigger new fetch/clone operations.
| if ! git diff --exit-code; then | ||
| git commit -m 'minor: Apply patches' . | ||
|
|
||
| # Changes were applied, so another compilation round may apply yet more | ||
| # changes. For performance reasons we perform incremental compilation, | ||
| # enabled using a misleading flag. (See | ||
| # https://issues.apache.org/jira/browse/MCOMPILER-209 for details.) | ||
| apply_patch '-Dmaven.compiler.useIncrementalCompilation=false' | ||
| elif [ "${extra_build_args}" != 'clean' ]; then | ||
| # No changes were applied. We'll attempt one more round in which all files | ||
| # are recompiled, because there are cases in which violations are missed | ||
| # during incremental compilation. | ||
| apply_patch 'clean' | ||
| fi |
There was a problem hiding this comment.
Yes, this is a bit awkward, but the added complexity seems worth the reduced runtime, at least for a build as large as this one. (Current runtime on my machine is ~11 minutes. Don't recall how long it is without this tweak; might do a new timing later.)
|
|
||
| # Perist or validate the warnings reported by Error Prone Support checks. | ||
| baseline_warnings="${integration_test_root}/${test_name}-expected-warnings.txt" | ||
| generated_warnings="$((grep -oP "(?<=^\\Q[WARNING] ${PWD}/\\E).*" "${validation_log_file}" | grep -P '\] \[' || true) | sort)" |
There was a problem hiding this comment.
Here and elsewhere, the use of grep -P likely doesn't make this script Macos-compatible; that's something to review separately.
|
Alright, probably the first of quite a lot of trial-and-error: /integration-test. |
|
Ah right, this might not work, given that the PR isn't merged yet. Will try something else later. |
|
I temporarily changed the default branch to be this PR's branch. So, take two: /integration-test |
|
That worked. I restored the default branch. |
Stephan202
force-pushed
the
sschroevers/integration-tests
branch
from
8757913 to
96bcb98
Compare
|
Validated in this build that artifacts are attached on build failure. |
The new `integration-tests/checkstyle-10.12.4.sh` script can be executed
locally or by adding a comment containing the string `/integration-test`
to a pull request.
The integration test comprises a shell script that:
1. Checks out a Checkstyle release tag.
2. Applies a small patch to enable Error Prone Support.
3. Runs the build in Error Prone patch mode, until no further changes
are applied.
4. Validates that:
- The build (including tests) still passes.
- The set of changes matches a pre-recorded diff.
- The set of Error Prone Support-emitted warnings matches a
pre-recorded list.
The script also has a `--sync` mode using which the expected diff and
warnings can be updated. Combined, the script makes it easy to assess
and track the impact of changes to Error Prone Support in relation to
the Checkstyle code base.
Over time this setup will be generalized to include integration tests
against other code bases.
This reverts commit a5f3b6f.
Stephan202
force-pushed
the
sschroevers/integration-tests
branch
from
2e3099a to
ba0d08e
Compare
|
Looks good. No mutations were possible for these changes. |
|
Alright, I think this PR is ready for review. Once merged we can start working on adding additional projects (something @oxkitsune has already worked on). |
|
Looks good. No mutations were possible for these changes. |
There was a problem hiding this comment.
Added a small commit.
How will the structure in integration-tests look like when we add more? And more versions? Do we want a directory per library? I see you changed some names and based on that I kind of assume you have already something in mind. Happy to think along, but for now going to call it a day :).
| # required to run Error Prone with Error Prone Support and (b) formatting the | ||
| # code using the same method by which it will be formatted after each | ||
| # compilation round. The initial formatting operation ensures that subsequent | ||
| # moifications can be rendered in a clean manner. |
There was a problem hiding this comment.
| # moifications can be rendered in a clean manner. | |
| # modifications can be rendered in a clean manner. |
|
|
||
| pushd "${project_root}" | ||
|
|
||
| # Make sure that Git is sufficient configured to enable committing to the |
There was a problem hiding this comment.
| # Make sure that Git is sufficient configured to enable committing to the | |
| # Make sure that Git is sufficiently configured to enable committing to the |
Right?
| # XXX: This "diff of diffs" also contains vacuous sections, introduced due to | ||
| # line offset differences. Try to omit those from the final output. | ||
| if ! diff -u "${expected_changes}" "${actual_changes}"; then | ||
| echo 'There are unexpected changes.' |
There was a problem hiding this comment.
Wouldn't it be great to output that to a file and upload that? I'd say that is of great value.
There was a problem hiding this comment.
That already happens; see the actions/upload-artifact goal. (And this comment.)
There was a problem hiding this comment.
I meant the "diff of the diffs". IIUC only the actual changes, actual logs and actual warnings are logged. While in cases of new changes you really want to see what the diff of the diffs is and inspect that.
There was a problem hiding this comment.
That diff-of-diffs is in the build log, and also easy to reproduce. Not opposed to adding this, but perhaps something for another PR. (I can't justify spending time on this right now.)
There was a problem hiding this comment.
Fine with deferring but I think its nice to add :).
| # moifications can be rendered in a clean manner. | ||
| git clean -fdx | ||
| git apply < "${integration_test_root}/${test_name}-init.patch" | ||
| git commit -m 'dependency: Introduce Error Prone Support' . |
There was a problem hiding this comment.
I see we are adhering to the guidelines of Checkstyle here, which makes me wonder, how do we want to generalize this script for the next runs? The setup is nice and seems to be written with a generalization in mind.
The reason I'm asking is that I started wondering if we always want to use the minor and dependency in the commit messages.
There was a problem hiding this comment.
Likely we'll want to make some compromises. Either by disabling additional checks or by making this parametric. While I already spent significant time on making the script reusable, there is certainly more to do. That's why the file is simply named integration-tests/checkstyle-10.12.4.sh ;)
There was a problem hiding this comment.
Ah that makes sense, sounds good for now :).
|
Kudos, SonarCloud Quality Gate passed!
|
|
Forgot to say something important; man this setup is amazing, cool stuff ! ✨ 💖 This will be so useful! |
There was a problem hiding this comment.
How will the structure in
integration-testslook like when we add more? And more versions? Do we want a directory per library? I see you changed some names and based on that I kind of assume you have already something in mind. Happy to think along, but for now going to call it a day :).
Good questions. I'm undecided about the value of tracking multiple versions per library, so perhaps dropping the versions from the file names make sense. Something to decide once we wish to update the tag against which we tag. W.r.t. whether to introduce subdirectories: also not sure; the current shared prefixes may suffice. The biggest thing to figure out is how to maximally reuse test script logic. I guess we'll figure it out as we go.
| # moifications can be rendered in a clean manner. | ||
| git clean -fdx | ||
| git apply < "${integration_test_root}/${test_name}-init.patch" | ||
| git commit -m 'dependency: Introduce Error Prone Support' . |
There was a problem hiding this comment.
Likely we'll want to make some compromises. Either by disabling additional checks or by making this parametric. While I already spent significant time on making the script reusable, there is certainly more to do. That's why the file is simply named integration-tests/checkstyle-10.12.4.sh ;)
| # XXX: This "diff of diffs" also contains vacuous sections, introduced due to | ||
| # line offset differences. Try to omit those from the final output. | ||
| if ! diff -u "${expected_changes}" "${actual_changes}"; then | ||
| echo 'There are unexpected changes.' |
There was a problem hiding this comment.
That already happens; see the actions/upload-artifact goal. (And this comment.)
| + "reported to standard out in plain format. Checkstyle requires a configuration " | ||
| + "XML file that configures the checks to apply.", | ||
| mixinStandardHelpOptions = true) | ||
| + // XXX: Don't reorder arguments to `picocli.CommandLine.Option#names`. |
There was a problem hiding this comment.
Or I don't get this sentence or it should be "from" instead of "to" 😋.
There was a problem hiding this comment.
One specifies arguments to a function. (In this case it's an annotation attribute, but still.)
| + // XXX: File PR to replace `.collect(toSet())` with `.collect(toCollection(HashSet::new))`. | ||
| + // XXX: Update `CollectorMutability` to recognize cases such as this one, where the created | ||
| + // collection is clearly modified. | ||
| + @SuppressWarnings("CollectorMutability") |
|
I'd say we can use "new feature" for this, or add a new label "integration-tests"/"integration-tests-framework"? I can file a PR such that it will get it's own section in the release notes. |
|
Good point. This is primarily relevant for contributors, with as a secondary benefit that hopefully the average quality of releases will go up. Perhaps the existing |
|
Fine by me, I just imagine that we will have quite some PRs related to this test framework so it could be convenient to have a label for it. Having it under improvement also works for now. |
Stephan202
added
chore
Suggested commit message: