Add passing tests and test duration #80

eugene-davis · 2018-07-31T15:51:58Z

Adds to the test data that is recorded the duration of each test.
In addition, adds passing tests to the data that is recorded for tests, so that you can get duration of all tests that were run.

Unit tests have been added.

jakub-bochenski · 2018-08-06T12:53:49Z

src/main/java/jenkins/plugins/logstash/persistence/BuildData.java

+
+      passedTests = new ArrayList<String>();
+      passedTestsWithDetail = new ArrayList<ExecutedTest>();
+      for (TestResult result : testResultAction.getPassedTests()) {


this isn't dry now, can you please refactor the logic into a helper method that will take in a collection of TestResult and fill a List and List?

I will address this shortly

This is done now, but it throws an unchecked/unsafe warning because I had to cast the return of getPassedTests/getFailedTests to List (the default return seems to be something like "capture # - TestResult"). Is there a better way to handle this or is it expected?

jakub-bochenski · 2018-08-06T12:57:27Z

src/main/java/jenkins/plugins/logstash/persistence/BuildData.java

@@ -67,15 +67,20 @@
  private final static Logger LOGGER = Logger.getLogger(MethodHandles.lookup().lookupClass().getCanonicalName());
  public static class TestData {
    private int totalCount, skipCount, failCount, passCount;
-    private List<FailedTest> failedTestsWithErrorDetail;
+    private List<ExecutedTest> failedTestsWithDetail;


This will change the JSON format in an incompatible way
The name has to stay the same for sake of backwards compatibility

Ok, I've left failed tests as FailedTest, but split passed tests to PassedTest (both of which inherit from the same class, ExecutedTest). That should keep the JSON format for the failed tests the same as before, but also avoid having passing tests under FailedTests.

The class name could have changed, it's only the field name that matters

Oh, sorry, that was obvious and I totally misread your first comment. I'll flip that back and remove the extra classes.

jakub-bochenski · 2018-08-06T12:58:09Z

@mwinter69 can you take a look too?

jakub-bochenski · 2018-08-07T09:23:41Z

src/main/java/jenkins/plugins/logstash/persistence/BuildData.java

+      }
+    }
+
+    public static class PassedTest extends ExecutedTest {


I fail to see why are those 2 classes needed?

Will fix, see above.

eugene-davis · 2018-08-07T15:41:02Z

src/main/java/jenkins/plugins/logstash/persistence/BuildData.java

+
+      passedTests = new ArrayList<String>();
+      passedTestsWithErrorDetail = new ArrayList<ExecutedTest>();
+      testListFill((List<TestResult>) testResultAction.getPassedTests(), passedTests, passedTestsWithErrorDetail);


Is there a smarter way to invoke this function than an unchecked cast?

jakub-bochenski · 2018-08-08T09:21:04Z

@eugene-davis sorry, I forgot to mention this at the begining

I'm worried about the amount of extra data this might generate. I assume for most builds the number of passing tests is far larger than the amount of failing tests.
I've seen many builds that execute 10k or more tests and the test output size was in GB range.

I think for this reasons sending passing tests output needs to be enabled via a configuration option.

eugene-davis · 2018-08-08T09:39:38Z

@jakub-bochenski I will take a look at adding this option. I'll plan to add it as disabled by default.

jakub-bochenski · 2018-08-08T10:14:15Z

OK, the biggest challenge will be the fact that now BuildData is created independently of the configuration options.

Alternatively you could wait for #71 - this would allow people to just remove the extra data with a groovy script.

eugene-davis · 2018-08-08T10:25:49Z

My thought was that something simple would work (but maybe it was naive - I haven't done much work with Jenkins plugins).
Something like this:
LogstashConfiguration configuration = LogstashConfiguration.getInstance();' 'if (configuration.isrecordingPassingTests()) {(set test)}

jakub-bochenski · 2018-08-08T10:31:37Z

Using singletons like that is generally considered an anti-pattern. I know current code is already doing that, but I'd prefer to add least not add more of it.

One way I'd suggest to refactor it is to use an abstract factory to create the BuildData, instead of using the constructor directly (as LogstashWriter does now).
The factory should probably take up most of the initData() logic as well as said "passed tests" switch.

eugene-davis · 2018-08-08T11:49:52Z

@jakub-bochenski I guess I'm not totally sure what you are suggestion with regards to the singleton.
As far as I can tell, the GlobalConfiguration class that the LogstashConfiguration extends upon exists as a singleton.
Is your idea to reduce the number of places that access the singelton, and pass the reference to it during construction (e.g. in an AbstractFactory)?

Edit: The short version of the question - Is the idea to not use singletons or use one in a different way?

jakub-bochenski · 2018-08-08T11:56:06Z

Is your idea to reduce the number of places that access the singelton, and pass the reference to it during construction (e.g. in an AbstractFactory)?

yes, this

mwinter69 · 2018-08-08T12:55:31Z

There should definitely also be the possibility to set this for the JobProperty, the notifier and the pipeline step individually overriding a centrally configured option.
One general problem I see when using it via JobProperty, globally enabled Consolelogfilter and the pipeline step is that we will send the data every time once it is available. Just imagine a post build step that is running after test data is available producing thousands of lines. This could could even create considerable load on the Mater due to serialization.
A possibility to avoid this might be to send this data only at the very end in a RunListener onCompleted or onFinalized call

eugene-davis · 2018-08-10T08:39:16Z

Ok on both, though I may move the initial refactoring into a separate branch and possibly PR to avoid having both the functional change and refactoring all go in at once.

…rding. Tests updated to remove references to getPassingTests

eugene-davis · 2018-08-10T12:46:32Z

The latest commit is non-final, just tracking some changes I am using on one of my Jenkins instances related to this PR.

mwinter69 · 2018-08-10T13:07:39Z

src/main/java/jenkins/plugins/logstash/LogstashConfiguration.java

@@ -82,6 +83,16 @@ public void setEnableGlobally(boolean enableGlobally)
    this.enableGlobally = enableGlobally;
  }

+  public void setrecordingPassingTests(boolean recordingPassingTests)


mwinter69 · 2018-08-10T13:08:42Z

src/main/resources/jenkins/plugins/logstash/LogstashConfiguration/config.jelly

@@ -7,6 +7,9 @@
 	    </f:entry>
 	    <f:entry title="Enable Globally" field="enableGlobally" description="This will not enable it for pipeline jobs.">
 	      <f:checkbox/>
+	    </f:entry>
+			<f:entry title="Enable Globally" field="recordingPassingTests" description="This will record all passing tests in addition to failing tests.">


Title is wrong

mwinter69 · 2018-08-10T13:40:18Z

src/main/java/jenkins/plugins/logstash/LogstashConfiguration.java

+    this.recordingPassingTests = recordingPassingTests;
+  }
+
+  public boolean isrecordingPassingTests()


mwinter69 · 2018-12-10T19:13:50Z

BTW, there are also skipped tests. This might be more interesting than the passed tests.

mwinter69 · 2018-12-10T19:23:09Z

src/main/java/jenkins/plugins/logstash/persistence/BuildData.java

+      }
+    }
+
+    private void testListFill(List<TestResult> testResults, List<String> testNames, List<ExecutedTest> testDetails) {


Make the signature of the method
List<? extends TestResult> testResults, List<String> testNames, ...
Then you don't need the type cast when you call the method

slxiao · 2019-10-16T08:11:03Z

@eugene-davis why this PR is closed? it's a good feature to have test duration in JSON report

jakub-bochenski · 2019-10-16T09:06:10Z

@slxiao you can check the discussion above for the remaining issues.
If you need this feature don't hesitate to submit a PR yourself

Add passing tests and test duration

ed3fc46

jakub-bochenski reviewed Aug 6, 2018

View reviewed changes

jakub-bochenski reviewed Aug 7, 2018

View reviewed changes

Reverted failedTestsWithErrorDetail name

82c1b7b

eugene-davis force-pushed the test-timing-capture branch from 9a93ff0 to 82c1b7b Compare August 7, 2018 09:59

eugene-davis added 2 commits August 7, 2018 17:33

Add testListFill function

58233d6

Change passedTestWithDetail to passedTestWithErrorDetail for consistency

8975d82

eugene-davis commented Aug 7, 2018

View reviewed changes

Fix failing test

685437e

Set empty List for passedTests when testResultAction is null

069e31c

Intermediate version with global setting to disable passing test reco…

51c51ea

…rding. Tests updated to remove references to getPassingTests

mwinter69 suggested changes Aug 10, 2018

View reviewed changes

mwinter69 reviewed Aug 10, 2018

View reviewed changes

Minor corrections to LogstashConfiguration

6932b51

eugene-davis force-pushed the test-timing-capture branch from 5fc18d2 to 6932b51 Compare August 10, 2018 15:11

mwinter69 reviewed Dec 10, 2018

View reviewed changes

eugene-davis closed this Apr 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add passing tests and test duration #80

Add passing tests and test duration #80

eugene-davis commented Jul 31, 2018

jakub-bochenski Aug 6, 2018

eugene-davis Aug 7, 2018

eugene-davis Aug 7, 2018

jakub-bochenski Aug 6, 2018

eugene-davis Aug 7, 2018

jakub-bochenski Aug 7, 2018

eugene-davis Aug 7, 2018

jakub-bochenski commented Aug 6, 2018

jakub-bochenski Aug 7, 2018

eugene-davis Aug 7, 2018

eugene-davis Aug 7, 2018

jakub-bochenski commented Aug 8, 2018

eugene-davis commented Aug 8, 2018

jakub-bochenski commented Aug 8, 2018

eugene-davis commented Aug 8, 2018

jakub-bochenski commented Aug 8, 2018

eugene-davis commented Aug 8, 2018 •

edited

Loading

jakub-bochenski commented Aug 8, 2018

mwinter69 commented Aug 8, 2018 •

edited

Loading

eugene-davis commented Aug 10, 2018

eugene-davis commented Aug 10, 2018

mwinter69 Aug 10, 2018

mwinter69 Aug 10, 2018

mwinter69 Aug 10, 2018

mwinter69 commented Dec 10, 2018

mwinter69 Dec 10, 2018

slxiao commented Oct 16, 2019

jakub-bochenski commented Oct 16, 2019

Add passing tests and test duration #80

Add passing tests and test duration #80

Conversation

eugene-davis commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakub-bochenski commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakub-bochenski commented Aug 8, 2018

eugene-davis commented Aug 8, 2018

jakub-bochenski commented Aug 8, 2018

eugene-davis commented Aug 8, 2018

jakub-bochenski commented Aug 8, 2018

eugene-davis commented Aug 8, 2018 • edited Loading

jakub-bochenski commented Aug 8, 2018

mwinter69 commented Aug 8, 2018 • edited Loading

eugene-davis commented Aug 10, 2018

eugene-davis commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwinter69 commented Dec 10, 2018

Choose a reason for hiding this comment

slxiao commented Oct 16, 2019

jakub-bochenski commented Oct 16, 2019

eugene-davis commented Aug 8, 2018 •

edited

Loading

mwinter69 commented Aug 8, 2018 •

edited

Loading