Skip to content

Conversation

@Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Dec 7, 2018

  • just running the tests for the moment

See JENKINS-55081.

Proposed changelog entries

  • Internal: correct test failing with Java 11: FilePathSEC904Test.isDescendant_regularFiles

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

@Wadeck Wadeck force-pushed the JENKINS-55081_xxx904Test_failing branch from be523fb to 87cba3d Compare December 10, 2018 10:06
@Wadeck Wadeck changed the title [DO NOT MERGE][JENKINS-55081] SEC-904 related test failing on java 11 [JENKINS-55081] SEC-904 related test failing on java 11 Dec 10, 2018
@batmat batmat self-requested a review December 10, 2018 10:45
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

LGTM as a test fix, but probably we need a user-facing changelog entry

@batmat
Copy link
Member

batmat commented Dec 10, 2018

I have checked the commits, and bear with me, I might have missed something. So IIUC, both tests and main code need to be changed because of the upstream relativize change in behavior.

To assess things stay fixed in Java 8, I think we should have the following:

  1. first commit only changes src/main/java code, not touching src/test/java yet
    • => we expect a PR build success on Java 8 (still ignore for now the failures with JDK11, given that's we're fixing)
  2. Once this shows we are still green on !Java-11, we also change the test so we remove failures from JDK11.

This way we do know that the code on Java 8 was still checked to work with the original tests it was written with.

WDYT?

@Wadeck
Copy link
Contributor Author

Wadeck commented Dec 10, 2018

@batmat I did not change the failing tests. I just added new ones.

@batmat
Copy link
Member

batmat commented Dec 10, 2018

Right indeed, I don't know why I thought I had seen replacements.

@batmat
Copy link
Member

batmat commented Dec 10, 2018

Ping @jenkinsci/java11-support BTW given this does relate to an issue only surfacing with Java 11.

@batmat batmat mentioned this pull request Dec 10, 2018
@batmat batmat added the needs-more-reviews Complex change, which would benefit from more eyes label Dec 10, 2018
@oleg-nenashev oleg-nenashev added the java11 Java 11 support in Jenkins label Dec 10, 2018
@batmat
Copy link
Member

batmat commented Dec 10, 2018

I think we have enough approvals to merge it as soon as, or not long after, the PR succeeds. I think we should merge this early so that other PRs stop being red for unrelated failures (and we easily see which one keep failing for actual reasons, that is)

@batmat batmat requested a review from daniel-beck December 10, 2018 12:28
@Wadeck
Copy link
Contributor Author

Wadeck commented Dec 10, 2018

Tests failing due to:

  • hudson.model.QueueTest.testBlockBuildWhenUpstreamBuildingLock <= flaky / time-dependent test
    • new SleepBuilder(5000), [...] new SleepBuilder(10000)
    • NOT displayed in BO Tests tab
  • hudson.util.AtomicFileWriterPerfTest.poorManPerformanceTestBed <= Baptiste is already on it

@Wadeck Wadeck added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Dec 10, 2018
@batmat
Copy link
Member

batmat commented Dec 10, 2018

I'm seeing only the latter failing:

image

Which indeed I am still analyzing in #3788

Let's merge this.

@batmat batmat merged commit c18bb84 into jenkinsci:master Dec 10, 2018
@batmat
Copy link
Member

batmat commented Dec 10, 2018

Filed as #3797

batmat added a commit to jglick/jenkins that referenced this pull request Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

java11 Java 11 support in Jenkins ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants