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

Test history refactoring and improvements #625

Merged
merged 85 commits into from
Jul 24, 2024

Conversation

mdealer
Copy link
Contributor

@mdealer mdealer commented Jul 9, 2024

This is an update of #446

Links to HPI iterations if you want to try it:

https://github.com/mdealer/junit-plugin/releases/

TL;DR:
Major performance and usability improvements to the test history file storage, history charts and test failure age computation.

image

Improve large JUnit test history performance and usability. RAM and CPU resources are better utilized to parse and cache the test results for faster retrieval.

JUnit SQL Storage plugin does not solve the problem fully as there are too many requests generated by the existing implementation.

To fully benefit from this, assign more CPU cores, e.g. 8+, and increase the Java heap size, e.g. -Xmx8g.

Changes:

  • Test result XML Files (junitResult.xml) are now parsed using an XML reader instead of XStream.
  • Test result history is loaded in parallel.
  • Test result history window (start and end build number) is now dynamic, allowing better navigation to past results via the webpage.
  • Test result history can now handle a much larger test and build counts in the thousands.
  • Test history graphs have been reimplemented to avoid build trend plugin and using ECharts directly for greater flexbility.
  • Test result age now scans back at most 25 builds without test results instead of resetting age at the first failed build
  • Test trend graph is shown more often on build page (some issues still remain in Jenkins core)
  • Some limited support for Dark Reader otherwise the graphs may turn unreadable
  • Added new parameter to junit pipeline step: keepTestNames for cases when your results already contain a namespace

Binary is now bigger due to a 3rd party math library for processing historical results.

The more CPU cores, the faster the result XML file parsing, the quicker the initial and subsequent history page load. The more heap size, the more test results will remain cached (using SoftReferences), the less CPU and Disk IO usage when browsing the results back and forth.

New parameter in 'junit' pipeline step: keepTestNames

Allow disabling the test name mangling when uploading results from multiple stages or parallel branches in a single build via 'keepTestNames' parameter. There was a change in the plugin a few years ago that basically broke the result URLs (e.g. if we are linking to the results from some external system), this is a way to undo this change.

def c = {
    node(null) {
        writeFile file: 'result.xml', text: '<testsuite><testcase time="2.6875" classname="MyTest" name="Run 0" assertions="3 total, 0 failed, 3 succeeded"/></testsuite>'
        junit keepTestNames: true, testResults: 'result.xml'
    }
}

parallel 'p1': c, 'p2': c

Related tickets that are potentially fixed

There various already reported issues that I also encountered on the way. This list must be double checked, but based on the code I looked at, there is a very high chance that most of the 'closed' tickets below are wrongly closed and this PR should improve the situation for them.

Status

TODOs:

  • 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
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Provide new tests --> already nearly fully covered by existing tests
  • Fix some existing tests
  • Sync with latest master
  • Provide escape hatches
  • Get rid of GPL dependency
  • Use colors from Design Library in history graphs (partially)
  • Clean up JS side for CSP

Testing done

Live 'tested' for over 1 year, also being resynced multiple times inbetween, so we are talking multiple Jenkins versions and thousands of builds with thousands of test results each.

The UI changes are functional, but I am not that familiar with Jenkins to make it fully clean. I think that should be done as part of another PR.

The existing tests should cover nearly all of the changes.

There is no 'before' screenshots because before it didn't work, the history page loaded forever.

See it in action

If you want to test it with large number of tests, then create a Pipeline Job, call it 'junit_test' and add this Groovy script into it:

def c = { target ->
    node(null) {
        //if (params.NUM?.toInteger() % 35 < Math.random() * 35) {
            def sb = new StringBuilder()
            sb.append('<testsuite>')
            def bn = (env.BUILD_NUMBER.toInteger() % 3) + 1
            def sets = []
            for (int i = 0; i < 20; ++i) {
                if (Math.random() > 0.2) {
                    sets.add(target + '.S' + i)
                }
            }
            
            def sr = Math.random()
            sets.each { set ->
                def failOnce = false
                int i = 0;
                for (; i < 200 + (env.BUILD_NUMBER.toInteger() * 0.01 * (Math.random() / 100 + 0.9)); ++i) {
                    def duration = env.BUILD_NUMBER.toInteger() / 10000 + Math.random() * 50 + Math.random() * Math.random() * 50 - 10000 * Math.random()
                    if (i % bn == 0 && sr < 0.3 && (sr < 0.1 ? !failOnce : true)) {
                        failOnce = true
                        sb.append("<testcase time=\"${duration}\" classname=\"${set}.SomeLongLongLongLongLongTestName_${i}\" name=\"Run 0\" assertions=\"3 total, 0 failed, 3 succeeded\">")
                        sb.append("<failure message=\"failure message ${i}\" type=\"Failure\"/>")
                        sb.append("</testcase>")
                    } else {
                        sb.append("<testcase time=\"${duration}\" classname=\"${set}.SomeLongLongLongLongLongTestName_${i}\" name=\"Run 0\" assertions=\"3 total, 0 failed, 3 succeeded\"/>")
                    }
                }
                int start = i
                for (; i < start + 40 + (env.BUILD_NUMBER.toInteger() * 0.01); ++i) {
                    sb.append("<testcase time=\"0\" classname=\"${set}.SomeLongLongLongLongLongTestName_${i}\" name=\"Run 0\" assertions=\"3 total, 0 failed, 3 succeeded\">")
                    sb.append("<skipped type=\"Skipped\" message=\"This test was disabled.\"/>")
                    sb.append("</testcase>")
                }
            }
            sb.append('</testsuite>')
            writeFile file: 'result.xml', text: sb.toString()
            junit keepTestNames: true, testResults: 'result.xml'
            archiveArtifacts 'result.xml'
        //}
    }
}
def p1c = { c('p1') }
def p2c = { c('p2') }
def m = [ 'p1': p1c, 'p2': p2c ]
parallel m

Then run the job for... thousands of times or as much as you like. Running takes some time, but navigating the results should be still fast. The NUM parameter is only required to have some builds without test results inbetween, you can use it if you want.

To speed things up, set up a second trigger job that executes this:

for (int i = 0; i < 500; ++i) {
    build quietPeriod: 0, wait: false, propagate: false, job: 'junit_test' //, parameters: [string(name: 'NUM', value: i.toString())]
}

Edgars Batna added 30 commits March 3, 2023 14:09
More logging.
Parallel history stream.
… tests.

Add timeout and build count limit.
Reduce trend chart loading times.
Display same count of builds in trend chart as in table.
Workaround for jQuery issue.
Support decimal duration.
Increase builds in view to 100.
Round maximum up if over 0.5.
Reuse the retrieved history for chart generator to avoid multiple requests.
Fix click on chart.
…it goofy).

Improve chart appearance.
Show both charts.
Add history size links.
Improve history page load time.
Add Total line.
More distinct line colors.
Improve overall appearance.
Increase age computation window to 25 to skip over builds without results.
Fix test compilation error.
Update POM.
Fix history chart layout with only few results.
@timja timja mentioned this pull request Jul 23, 2024
6 tasks
mdealer pushed a commit to mdealer/junit-attachments-plugin that referenced this pull request Jul 23, 2024
mdealer pushed a commit to mdealer/junit-attachments-plugin that referenced this pull request Jul 23, 2024
@mdealer
Copy link
Contributor Author

mdealer commented Jul 23, 2024

I discovered that properties are not loaded properly, I am fixing it (this was added by another PR in the last year or so and I forgot to implement the parsing).

I will also see if I can implement a test for writing+parsing.

@timja
Copy link
Member

timja commented Jul 24, 2024

Any final concerns or shall we ship?

@timja
Copy link
Member

timja commented Jul 24, 2024

Going to ship now to benefit from some of @mdealer's availability if required.

@timja timja merged commit 72cf99b into jenkinsci:master Jul 24, 2024
16 checks passed
@timja
Copy link
Member

timja commented Jul 24, 2024

Thanks @mdealer!

Released in https://github.com/jenkinsci/junit-plugin/releases/tag/1279.v72cf99b_25c43

MarkEWaite added a commit to MarkEWaite/docker-lfs that referenced this pull request Jul 24, 2024
jenkinsci/junit-plugin#625 includes history
improvemnts and performance improvements.  Especially helpful for large
JUnit results.
<dependency>
<groupId>ca.umontreal.iro.simul</groupId>
<artifactId>ssj</artifactId>
<version>3.3.1</version>
Copy link
Member

Choose a reason for hiding this comment

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

This was outdated at the time of integration. I think I have managed to upgrade it in #628.

<dependency>
<groupId>com.pivovarit</groupId>
<artifactId>parallel-collectors</artifactId>
<version>2.6.1</version>
Copy link
Member

Choose a reason for hiding this comment

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

This is up-to-date, but Renovate is trying to upgrade it to version 3.x in e.g. #629, which won't work because 3.x requires Java 21 or newer. Can we update the Renovate configuration to exclude 3.x until we are ready to require Java 21 or newer? That way, we will continue to get updates to version 2.x.

@basil
Copy link
Member

basil commented Sep 6, 2024

Breaks org.jenkinsci.plugins.parallel_test_executor.ParallelTestExecutorTest.unloadableTestResult with

java.lang.AssertionError: 

Expected: a string containing "Build #2 has no loadable test results (supposed count 1), skipping"
     but: was "Started
[Pipeline] Start of Pipeline
[Pipeline] splitTests
Using build #2 as reference
1 test classes (0ms) divided into 1 sets. Min=0ms, Average=0ms, Max=0ms, stddev=0ms
[Pipeline] End of Pipeline
Finished: SUCCESS
"
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.jvnet.hudson.test.JenkinsRule.assertLogContains(JenkinsRule.java:1570)
        at org.jenkinsci.plugins.parallel_test_executor.ParallelTestExecutorTest.unloadableTestResult(ParallelTestExecutorTest.java:124)
        at java.base/java.lang.reflect.Method.invoke(Method.java:569)
        at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:655)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.lang.Thread.run(Thread.java:840)

return r;
}

static ConcurrentHashMap<String, SoftReference<TestResult>> resultCache = new ConcurrentHashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect FTR: Jenkins plugins should never keep state in static fields. Rather use instance fields of some @Extension. Noticed because of a test failure in jenkinsci/parallel-test-executor-plugin#296 where we try to simulate a Jenkins restart without using RealJenkinsRule and thus a new JVM.

Copy link
Contributor Author

@mdealer mdealer Sep 9, 2024

Choose a reason for hiding this comment

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

I am not that familiar with Jenkins extension mechanism, I just assumed that it's OK because

  • it is in the gray, between the state of the plugin and that of the file system
  • plugins cannot be unloaded

Maybe there is a good example on how to do it?

Copy link
Member

Choose a reason for hiding this comment

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

In practice this mainly causes trouble for functional tests. If there is not already an @Extension which would be a natural home for the state, you can simply add one

@Extension public static final class TheCache {
    static TheCache get() {
        return ExtensionList.lookupSingleton(TheCache.class);
    }
    private final Cache<Whatever, Else> data = Caffeine.newCache().orSomething();
    // etc.
}

jglick added a commit to jglick/parallel-test-executor-plugin that referenced this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants