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

[JENKINS-34217] add option to disable historical statistics gathering… #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

XavierRaynaud
Copy link

… for test results

This change adds a new job configuration property that, when enabled, disables
the display of the test trend chart and prevents the plugin from even calculating
how long a test has been failing for, thus saving the work of digging up historical
records.
Note that this commit is squashing the 3 commits initially devloped by Jim Carrothers.
It also fixes h.t.j.JobTestResultDisplayProperty$DescriptorImpl.getDisplayName() return value.

@timja
Copy link
Member

timja commented Jul 30, 2020

Can you please take a look at the merge conflicts,

Is this still needed after #134,

It no longer blocks page render, instead it is loaded async

@jglick
Copy link
Member

jglick commented Aug 19, 2020

Also #142 should obviate the need for such workarounds.

@qlihdunn
Copy link

qlihdunn commented Jul 9, 2021

This bug is killing our CI - Interested to help merge. Can anyone help or get me started - if I can get time allocated?

… for test results

This change adds a new job configuration property that, when enabled, disables
the display of the test trend chart and prevents the plugin from even calculating
how long a test has been failing for, thus saving the work of digging up historical
records.
Note that this commit is squashing the 3 commits initially devloped by Jim Carrothers.
It also fixes h.t.j.JobTestResultDisplayProperty$DescriptorImpl.getDisplayName() return value.
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.

I've done a sanity review and not tested it locally, it won't work as is.

I would strongly suggest people check out https://github.com/jenkinsci/junit-sql-storage-plugin if this is causing an issue for them.

@XavierRaynaud / @qlihdunn what is the reason you want this feature? the issue talks about a specific gerrit feature, is your use-case the same?

@@ -156,7 +156,7 @@ private TestResultActionIterable createBuildHistory(Run<?, ?> lastCompletedBuild
@Deprecated
public void doTrend( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException {
AbstractTestResultAction a = getLastTestResultAction();
if(a!=null)
if(a!=null && a.shouldCalculatePreviousResults())
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 not used anymore

@@ -25,5 +25,31 @@ THE SOFTWARE.
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout"
xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt" xmlns:local="local" xmlns:c="/charts">
<c:trend-chart it="${from}" title="${%Test Result Trend}" enableLinks="true"/>
<j:set var="tr" value="${action.lastTestResultAction}" />
Copy link
Member

Choose a reason for hiding this comment

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

incorrect merge conflict resolution

@Override
public JobTestResultDisplayProperty newInstance(StaplerRequest req, JSONObject formData) {
if (formData.isNullObject()) return null;
return new JobTestResultDisplayProperty(formData.getBoolean("junitsettings-disableHistoricalResults"));
Copy link
Member

Choose a reason for hiding this comment

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

field should be stored on the descriptor I think and then use data binding instead of retrieving from formData

*
*/
public class JobTestResultDisplayProperty extends JobProperty<Job<?, ?>> {
private final boolean disableHistoricalResults;
Copy link
Member

Choose a reason for hiding this comment

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

use spaces not tabs please to be consistent with elsewhere

@XavierRaynaud
Copy link
Author

Hi @timja,

Many thanks for your comment.
Frankly speaking, I just retrieved Jim Carrothers's patch, and tried to apply it. It seems that it requires a bit more work :)

The reason I want this feature is the following:
(1) I have jenkins jobs with a huge amount of junit test records.
(2) Moreover, some tests in our jobs are lazyly run (they are performed only when corresponding code has been modified).

Because of (1), trend chart, statistics, and browsing test results tooks a huge time.
Because of (2), trend chart, statistics, and browsing in test history is often meaningless.

Therefore, I would like to cut down all history computation for such jobs.

@timja
Copy link
Member

timja commented Aug 2, 2021

(1) I have jenkins jobs with a huge amount of junit test records.

https://github.com/jenkinsci/junit-sql-storage-plugin should help with this.

and it should make it so that 2 is not such a problem

@XavierRaynaud
Copy link
Author

Ok, thanks for the info.

I had a look on the code, and did not see anything about DELETE statements.
Does it means that junit results stored in database are never deleted, even when run are deleted ?

@timja
Copy link
Member

timja commented Aug 2, 2021

Ok, thanks for the info.

I had a look on the code, and did not see anything about DELETE statements.
Does it means that junit results stored in database are never deleted, even when run are deleted ?

not currently deleted as they are also used for historical test results, run auto-rotation doesn't mean that the test results are no longer interesting.

but some form of cleaning should be implemented

@XavierRaynaud
Copy link
Author

Ok. thanks.

As I said before, I've a huge number of junit tests.
Moreover, stdout/stderr of such tests can be huge too...

Therefore, I can't switch to a database if the policy is to keep results forever.

@timja
Copy link
Member

timja commented Aug 5, 2021

Ok. thanks.

As I said before, I've a huge number of junit tests.
Moreover, stdout/stderr of such tests can be huge too...

Therefore, I can't switch to a database if the policy is to keep results forever.

It now deletes them when the job or build is deleted https://github.com/jenkinsci/junit-sql-storage-plugin/releases/tag/99.v79279ea1504d

@XavierRaynaud
Copy link
Author

Wow cool ! Thanks a lot. I will test it asap. I think we can close this pull request.

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.

5 participants