Skip to content

Conversation

@basil
Copy link
Member

@basil basil commented Oct 4, 2022

Problem

We recently added support for JUnit 5 in #6657. This exposed us to SUREFIRE-2087, a bug that affects even JUnit 4 based parameterized tests that are executed with JUnit 5 Vintage, as demonstrated in this minimal reproducible (MRE). Our exposure to this bug meant that a new test failure introduced in #7052 went unnoticed, causing SECURITY-2886.

Solution

The Maven developers remain unresponsive to SUREFIRE-2087, but the problem can be worked around by simply not using rerunFailingTestsCount. We do not use it in plugin-pom or pom, only in the test/ module of Jenkins core. This PR removes it from there, thereby eliminating our exposure to the problem.

Implementation

There are two main considerations with regard to implementation: will this negatively affect PR builds, and will this negatively affect releases?

  1. Regarding the first, it certainly would have negatively affected PR builds several weeks ago, which is why I did not make this change at that time. That is because many of the tests in our test suite are flaky. But I have been carefully studying build and release logs and fixing the worst offenders. As of today is it not uncommon to get builds of the main branch with no flakes at all, and while I am aware of a few flakes that still show up occasionally, I either have fixes in progress for them or have made changes to collect additional debugging information in anticipation of a fix. Regardless, I think the current rate of flakiness is lower than the current rate of infrastructure failures due to node disconnections, and we have tolerated the latter for some time already, so my answer to the first question is: no, this will not (overly) negatively affect PR builds.
  2. Regarding the second, both this week's release build and last week's release build had no flakes. And I also went through the logs for the past two months' worth of releases and ensured that any flakes that showed up in the past are ones that I have already fixed in the past few weeks. So there should not be impact to releases. Still, in the worst-case scenario that a flake does end up blocking a release, we could always turn rerunFailingTestsCount back on in the jenkins-release profile. I hope we do not have to do this, but in the worst-case scenario we would not be out of options.

Testing done

I have been running many PR builds and have observed far fewer flakes than I did several weeks ago. While I am aware of a few flakes that still show up occasionally, I either have fixes in progress for them or have made changes to collect additional debugging information in anticipation of a fix. Either way, I do not view the remaining flakes as having a prohibitively negative impact on development should they become reported as failures rather than retried.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@basil basil added the skip-changelog Should not be shown in the changelog label Oct 4, 2022
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for all the effort you've gone to to track down these flakey tests, it's really appreciated!

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks very much!

@MarkEWaite MarkEWaite added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 4, 2022
@timja
Copy link
Member

timja commented Oct 4, 2022

Regardless, I think the current rate of flakiness is lower than the current rate of infrastructure failures due to node disconnections

Hopefully improved with #7220

@basil basil merged commit de8256a into jenkinsci:master Oct 5, 2022
@basil basil deleted the surefire-rerunFailingTestsCount branch October 5, 2022 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants