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

If we are catching PomExecutionException do not claim SUCCESS #1362

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

basil
Copy link
Member

@basil basil commented Jul 29, 2022

@basil basil requested review from jglick, jetersen and timja July 29, 2022 18:05
Comment on lines +109 to 112
elif grep -q -F -e '<status>COMPILATION_ERROR</status>' pct-report.xml; then
echo 'PCT failed with compilation error' >&2
cat pct-report.xml
exit 1
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines +114 to +119
#
# Previous versions of PCT claimed that there were test failures even when no tests had been
# run at all. While it is possible that current versions of PCT no longer exhibit this
# pathology, we err on the side of caution and check anyway.
#
echo 'PCT marked failed, checking to see if that is due to a failure to run tests at all' >&2
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about deleting this logic entirely, but I failed to convince myself that it was not needed, so better safe than sorry.

Comment on lines -112 to -125
check=false
levels=$(echo "$t" | tr / '\n' | wc -l)
short_name=$(echo "$t" | tr / '\n' | grep -v ^target$ | tail -1)
if [[ $levels -lt 4 ]]; then
# Single-module project or root module of multi-module
# project: always check.
check=true
elif [[ $PLUGINS =~ $short_name ]]; then
# Submodule of multi-module project: only check if the
# directory name of the submodule is a substring of the
# list of plugins we are testing.
check=true
fi
if $check && [[ -f "${t}/test-classes/InjectedTest.class" ]] && [[ ! -f "${t}/surefire-reports/TEST-InjectedTest.xml" ]] && [[ ! -f "${t}/failsafe-reports/TEST-InjectedTest.xml" ]]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

I really hated this code (which I wrote in a previous PR) because it is quite convoluted and fragile. I was hoping to get rid of it entirely, as it is not needed in many cases where it was previously needed as of newer versions of PCT, but it is still needed in one case, described in the comments above. That case is simple to work around with a simple rm, so I opted to trade one complex and fragile workaround for a simpler (but still fragile) workaround. This I call progress.

@@ -41,7 +41,7 @@ for LINE in $LINEZ; do
done

# TODO find a way to encode this in some POM so that it can be managed by Dependabot
version=1178.vbef3c43d0e69
version=1180.v0e40e65a81e1
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Glad to see this.

@basil basil enabled auto-merge (squash) July 29, 2022 18:45
@basil basil merged commit 8b39ae4 into jenkinsci:master Jul 29, 2022
@basil basil deleted the PomExecutionException branch July 29, 2022 19:54
@jetersen jetersen added the chore Reduces future maintenance label Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Reduces future maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build passes even after PCT fails to run on a plugin
3 participants