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

Adjust/enhance some tests #113

Merged
merged 3 commits into from
Sep 5, 2022

Conversation

centic9
Copy link
Member

@centic9 centic9 commented Sep 4, 2022

Print out more information on failure
Mention that some tests need to run with installed agent
Add a few more tests
Some classes are not available when running tests on Windows

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • [o] Link to relevant issues in GitHub or Jira
  • [o] Link to relevant pull requests, esp. upstream and downstream changes
  • [o] Ensure you have provided tests - that demonstrates feature works or fixes the issue

Print out more information on failure
Mention that some tests need to run with installed agent
Add a few more tests
Some classes are not available when running tests on Windows
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 so much for improving the tests. One of the tests fails on my Linux environment. The same test passes on the master branch. Could you investigate and resolve the test failure?

assertNotNull("No file record for file=" + tempFile + " found", findFileRecord(tempFile));

assertThat("Did not have the expected type of 'marker' object: " + obj,
Copy link
Contributor

Choose a reason for hiding this comment

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

This assertion fails when I run the test on my Red Hat Enterprise Linux 8 computer with Eclipse Adoptium Java 11.0.16.1 and Apache Maven 3.8.6 using the command line:

$ mvn -version
Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)
Maven home: /home/mwaite/tools/apache-maven-3.8.6
Java version: 11.0.16.1, vendor: Eclipse Adoptium, runtime: /home/mwaite/tools/jdk-11.0.16.1+1
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "4.18.0-372.19.1.el8_6.x86_64", arch: "amd64", family: "unix"
$ mvn clean verify
[ERROR] Failures:
[ERROR]   FileDemo.openCloseFileLines:262 Did not have the expected type of 'marker' object: sun.net.www.protocol.jar.URLJarFile@7b69c6ba
Expected: an instance of java.nio.channels.SeekableByteChannel
     but: <sun.net.www.protocol.jar.URLJarFile@7b69c6ba> is a sun.net.www.protocol.jar.URLJarFile
[INFO]
[ERROR] Tests run: 14, Failures: 1, Errors: 0, Skipped: 0

The same failure is seen with JDK 11.0.16 that is available with RHEL 8 as an RPM package.

The same command line, operating system, and Java version run the test successfully from the master branch.

The failure is not seen with JDK 17.0.4.1 with this pull request.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very strange flakiness that is likely not introduced by my changes.

It works at times, but fails with this sometimes. Seems to be some race condition with some jar-file loading in other threads or similar.

I can try to narrow it down a bit more.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be a close() via a Finalizer done concurrently sometimes.

I now exclude closing of URLJarFile in the test to avoid this flakiness.

This seems to happen concurrently via finalizers and did lead to flaky tests
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.

I've confirmed that the machine that previously showed consistent failures did not show any failure with 10 runs of mvn clean verify. Also confirmed no failures with 10 runs on a second machine. Thanks very much!

Optional comment based on formatting. Whether or not that comment is applied, I plan to merge this after 24 hours unless others raise an objection.

Co-authored-by: Mark Waite <[email protected]>
@MarkEWaite MarkEWaite merged commit a46983d into jenkinsci:master Sep 5, 2022
@centic9 centic9 deleted the various_test_improvements branch September 20, 2022 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants