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

Introduce generic run-integration-test.sh script #1141

Merged
merged 4 commits into from
Aug 7, 2024

Conversation

rickie
Copy link
Member

@rickie rickie commented Apr 17, 2024

This PR aims to split out some of the changes from #894 to minimize the diff of that PR.
It's introducing too many changes to review at once. After we merged this PR, it'll be easier to review the other.

Credits to @oxkitsune for these changes!

@rickie rickie added this to the 0.17.0 milestone Apr 17, 2024
@rickie rickie force-pushed the rossendrijver/extract_running_integration_test branch from b61b4b5 to 5d15247 Compare April 17, 2024 06:09
@rickie
Copy link
Member Author

rickie commented Apr 17, 2024

/integration-test

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.

1 similar comment
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

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

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.

Clean 🧹

@rickie rickie force-pushed the rossendrijver/extract_running_integration_test branch from 5d15247 to be79b3b Compare May 24, 2024 11:24
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.

@rickie rickie force-pushed the rossendrijver/extract_running_integration_test branch from be79b3b to 0c9e563 Compare June 11, 2024 10:12
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

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@rickie rickie force-pushed the rossendrijver/extract_running_integration_test branch from 0c9e563 to ee1b4b4 Compare July 14, 2024 12:12
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.

@Stephan202 Stephan202 modified the milestones: 0.17.0, 0.18.0 Jul 20, 2024
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.

Rebased and added a commit. One question remaining!

|| (
git -C "${project_root}" fetch --depth 1 "${repository}" "${revision}" \
&& git -C "${project_root}" checkout --force FETCH_HEAD \
&& (git -C "${project_root}" tag "${revision}" --message "${revision}" || true)
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 understand why --message "${revision}" is now necessary here/is there an easy way for me to see what breaks if this is omitted? 👀

(Looking at git tag --help, I suspect this has to do with a tag.gpgSign user setting.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I have the gpgSign setup. Before adding this, I was required to pass in a message (via vim in the commandline) myself before it would continue with running. But instead we can also say --no-sign perhaps?

Hmm, I ran it again without the --message "{revision}" and it works for me... I have a new PC setup since the last time I worked on this, so maybe it's slightly different now?

Will drop it.

Comment on lines 167 to 171
# are behavior preserving. Some tests are skipped:
# - The `metadataFilesGenerationAllFiles` test is skipped because it 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.
Copy link
Member

Choose a reason for hiding this comment

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

This documentation must now live elsewhere.

mvn ${shared_build_flags} \
clean package \
-Derror-prone.configuration-args="${error_prone_validation_flags}" \
${validation_mvn_flags} \
Copy link
Member

Choose a reason for hiding this comment

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

Above we use shared_build_flags, so I'd say that this property should then be names validation_build_flags.

;;
esac

# XXX: Configure Renovate to manage the AssertJ version declared here.
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
# XXX: Configure Renovate to manage the AssertJ version declared here.

repos_root="${integration_test_root}/.repos"

if [ "${#}" -ne 11 ]; then
>&2 echo "Usage $(basename "${0}") [TestName] [Project] [Repository] [Revision] [BuildFlags] [AdditionalSourceDirectories] [PatchFlags] [ValidationEpFlags] [ValidationMvnFlags] [DoSync] [ReportDirectory]"
Copy link
Member

Choose a reason for hiding this comment

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

Colon missing after "Usage". 👀

The parameters aren't optional, so we should use angled brackets, rather than square brackets.

Comment on lines 17 to 27
test_name="${1}"
project="${2}"
repository="${3}"
revision="${4}"
build_flags="${5}"
additional_src_directories="${6}"
patch_flags="${7}"
validation_ep_flags="${8}"
validation_mvn_flags="${9}"
do_sync="${10}"
report_directory="${11}"
Copy link
Member

Choose a reason for hiding this comment

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

The naming here isn't very consistent; will propose some things.

Comment on lines +73 to +79
find "${error_prone_support_root}" \
-path "*/META-INF/services/com.google.errorprone.bugpatterns.BugChecker" \
-not -path "*/error-prone-experimental/*" \
-not -path "*/error-prone-guidelines/*" \
-print0 \
Copy link
Member

Choose a reason for hiding this comment

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

Indentation partially corrected here, but not fully. Should do the same below.

Comment on lines 14 to 15
do_sync="$([ "${#}" = 0 ] || [ "${1:-}" != '--sync' ] || echo 1)"
report_directory="$([ "${#}" = 0 ] || ([ -z "${do_sync}" ] && echo "${1}") || ([ "${#}" = 1 ] || echo "${2}"))"
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 partially deduplicate this ^ logic too; will propose something.

@Stephan202 Stephan202 force-pushed the rossendrijver/extract_running_integration_test branch from ee1b4b4 to 03d1a70 Compare August 4, 2024 16:18
@Stephan202 Stephan202 added chore A task not related to code (build, formatting, process, ...) and removed improvement labels Aug 4, 2024
Copy link

github-actions bot commented Aug 4, 2024

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.

@Stephan202
Copy link
Member

Suggested commit message:

Introduce generic `run-integration-test.sh` script (#1141)

This new script contains reusable logic extracted from
`integration-tests/checkstyle.sh`, facilitating the introduction of additional
integration tests.

Copy link

github-actions bot commented Aug 7, 2024

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.

@rickie rickie requested a review from Stephan202 August 7, 2024 13:42
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.

Let's go! 🚀

@Stephan202 Stephan202 force-pushed the rossendrijver/extract_running_integration_test branch from b837751 to 342bf12 Compare August 7, 2024 18:07
Copy link

sonarqubecloud bot commented Aug 7, 2024

Copy link

github-actions bot commented Aug 7, 2024

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.

@Stephan202 Stephan202 merged commit c322ea1 into master Aug 7, 2024
15 checks passed
@Stephan202 Stephan202 deleted the rossendrijver/extract_running_integration_test branch August 7, 2024 21:12
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