[CI] Produce junit test reports #15281

spalger · 2017-11-30T00:56:52Z

This PR extends the mocha, jest, and functional test runner implementations to produce JUnit test reports. These reports allow Jenkins to track the results of individual tests and should help us understand which tests fail regularly, and which are being flaky.

To view the test results, navigate to the console output for the specific job like always, but to get a test-by-test breakdown click the new "Test Result" link in the left hand nav

azasypkin

LGTM, thanks for working on this!

azasypkin · 2017-12-04T12:20:32Z

src/dev/mocha/junit_reporter.js

+    suite.startTime = Date.now();
+  });
+
+  runner.on('test', (suite) => {


nit: suite --> test?

azasypkin · 2017-12-04T12:20:54Z

src/dev/mocha/junit_reporter.js

+    suite.startTime = Date.now();
+  });
+
+  runner.on('test end', (suite) => {


nit: suite --> test?

azasypkin · 2017-12-04T12:43:29Z

src/dev/mocha/junit_reporter.js

+    const segments = [...directory.split(pathSep), filenameWithoutExt];
+
+    return segments
+      .filter(seg => seg && !PATH_SEGMENTS_TO_IGNORE.includes(seg))


note: it's probably a matter of personal taste, but having "raw" core_plugins/kibana/common/field_formats/types/__tests__/truncate.js instead of
CorePlugins/Kibana/Common/FieldFormats/Types/Truncate seems to be easier to understand and map to actual test file.

That's fair, the issue I primarily had with using paths was that they can't include . characters as Jenkins treats everything to the left of the last . as the top level package, and everything following the . as the classname. To work around this I was replacing . with ∙, so the path looked similar but they weren't really paths anymore... Do you think you'd prefer the paths with . replaced?

That's fair, the issue I primarily had with using paths was that they can't include . characters as Jenkins treats everything to the left of the last . as the top level package, and everything following the . as the classname. To work around this I was replacing . with ∙, so the path looked similar but they weren't really paths anymore...

Hmm, I see, thanks for the details. Even though both options work for me, I still would prefer to be as close to real paths as possible. I'm going to defer that to you and @kjbekkelund as I don't have a strong opinion on that :)

I went ahead and updated the jest and mocha reporters to use relative file paths for the test "classname", will post comparisons when they're done running.

class named derived from the file path:

file path with '.' replaced with '·':

Thanks for the screenshots, I still prefer the latter one personally, not a big deal though :)

azasypkin · 2017-12-04T13:24:41Z

src/functional_test_runner/lib/mocha/console_reporter/console_reporter.js

@@ -24,6 +26,16 @@ export function ConsoleReporterProvider({ getService }) {
      runner.on('test end', this.onTestEnd);
      runner.on('suite end', this.onSuiteEnd);
      runner.on('end', this.onEnd);
+
+      if (config.get('junit.enabled') && config.get('junit.reportName')) {
+        new MochaJunitReporter(runner, {


nit: can't MochaJunitReporterbe just a function like setupMochaJunitReporter? Is there any reason why it should be a constructor function?

I originally added this as a standard mocha reporter, which is instantiated with new, but no reason for it anymore

azasypkin · 2017-12-04T13:27:09Z

src/dev/mocha/junit_reporter.js

+          .dat(inspect(test.err));
+      }
+
+      if (isTestPending(test)) {


nit: else if maybe?

azasypkin · 2017-12-04T13:52:10Z

src/functional_test_runner/lib/mocha/console_reporter/console_reporter.js

@@ -2,17 +2,19 @@ import { format } from 'util';

 import Mocha from 'mocha';

+import { MochaJunitReporter } from '../../../../dev/mocha/junit_reporter';
 import * as colors from './colors';
 import * as symbols from './symbols';
 import { ms } from './ms';
 import { writeEpilogue } from './write_epilogue';

 export function ConsoleReporterProvider({ getService }) {


optional nit: it isn't just a consolereporter anymore, maybe we can have a better name for this reporter, it's not a big deal though.

azasypkin · 2017-12-04T14:33:41Z

tasks/config/simplemocha.js

  },
  all: {
    src: [
-      'test/mocha_setup.js',
+      require.resolve('../../src/babel-register'),


question: just for my understanding, do we need require.resolve('../../src/babel-register') here? I thought it's either loaded from mocha.opts, mocha.js script or from main Gruntfile....

grunt-simplemocha imports mocha and interacts with it directly, so neither mocha.opts or scripts/mocha.js would be called. Now that I typed that out I realize the Gruntfile is loading babel-register already so this is unnecessary.

azasypkin · 2017-12-04T14:49:52Z

tasks/config/karma.js

+        ].join(' '),
+        classNameFormatter: (browser, result) => {
+          const rootSuite = result.suite[0] || result.description;
+          return `Browser Unit Tests.${rootSuite.split('.').join('·')}`;


nit: .split('.').join('·') ---> .replace(/\./gi, '·') maybe (just to make intention clearer)?

I think I got in this habit when reading about the efficiency of replace... but readability > micro-performance-improvements

azasypkin · 2017-12-04T14:53:51Z

src/dev/jest/junit_reporter.js

+const ROOT_DIR = dirname(require.resolve('../../../package.json'));
+
+/**
+ * Jest reporter that that produces JUnit report when running on CI


typo: redundant that

kimjoar

This looks great! LGTM

* [mocha] use custom reporter for legible results in jenkins * [jest] use custom result processor for legible results in jenkins * [karma] enable junit output on CI * [mocha/junitReporter] accept rootDirectory as configuration * [jest/reporter] use reporters option added in jest 20 * [toolingLog] remove black/white specific colors * [dev/mocha/junit] no reason for junit to be a "reporter" * typos * [dev/mocha/junit] use else if * [karma/junit] use string#replace for explicitness * [junit] use test file path as "classname" * [ftr/mocha] no longer a "console" specific reporter

* [CI] Produce junit test reports (#15281) * [mocha] use custom reporter for legible results in jenkins * [jest] use custom result processor for legible results in jenkins * [karma] enable junit output on CI * [mocha/junitReporter] accept rootDirectory as configuration * [jest/reporter] use reporters option added in jest 20 * [toolingLog] remove black/white specific colors * [dev/mocha/junit] no reason for junit to be a "reporter" * typos * [dev/mocha/junit] use else if * [karma/junit] use string#replace for explicitness * [junit] use test file path as "classname" * [ftr/mocha] no longer a "console" specific reporter * [dev/jest] fix relative path to babel options

spalger · 2017-12-06T02:41:49Z

5.6: 5c5e03a
6.0: d16ff7a
6.1: 0969d3d
6.2/6.x: b4bd640

spalger added the WIP Work in progress label Nov 30, 2017

spalger force-pushed the implement/junit-test-reports branch 27 times, most recently from a6eb638 to 82678a1 Compare December 1, 2017 07:33

spalger added review v5.6.5 labels Dec 1, 2017

spalger added 2 commits December 1, 2017 18:45

[mocha/junitReporter] accept rootDirectory as configuration

59f4f2f

[jest/reporter] use reporters option added in jest 20

4cef043

spalger force-pushed the implement/junit-test-reports branch from 11d2eb5 to 4cef043 Compare December 2, 2017 01:50

[toolingLog] remove black/white specific colors

c593482

azasypkin approved these changes Dec 4, 2017

View reviewed changes

spalger added 6 commits December 4, 2017 14:55

[dev/mocha/junit] no reason for junit to be a "reporter"

1b38477

typos

0cce300

[dev/mocha/junit] use else if

31d0f7c

[karma/junit] use string#replace for explicitness

dd51595

[junit] use test file path as "classname"

954994c

[ftr/mocha] no longer a "console" specific reporter

4884fe6

kimjoar approved these changes Dec 5, 2017

View reviewed changes

spalger merged commit f71ec29 into elastic:master Dec 6, 2017

spalger mentioned this pull request Dec 6, 2017

[6.x] [CI] Produce junit test reports (#15281) #15437

Merged

This was referenced Dec 6, 2017

[6.1] [CI] Produce junit test reports (#15281) #15439

Merged

[6.0] [CI] Produce junit test reports (#15281) #15441

Merged

spalger mentioned this pull request Dec 6, 2017

[5.6] [CI] Produce junit test reports (#15281) #15442

Merged

spalger deleted the implement/junit-test-reports branch December 6, 2017 02:41

This was referenced Feb 14, 2018

[test:server] fall-back to babel/register script if mocha_setup not available elastic/kibana-plugin-helpers#68

Merged

task 'test:server' failed after upgrade to Kibana 5.6.7 elastic/kibana-plugin-helpers#67

Closed

spalger mentioned this pull request Feb 13, 2019

Fix plugin-helpers test:server task #31034

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Produce junit test reports #15281

[CI] Produce junit test reports #15281

spalger commented Nov 30, 2017 •

edited

Loading

azasypkin left a comment

azasypkin Dec 4, 2017

azasypkin Dec 4, 2017

azasypkin Dec 4, 2017

spalger Dec 4, 2017 •

edited

Loading

azasypkin Dec 4, 2017

spalger Dec 4, 2017

spalger Dec 4, 2017

azasypkin Dec 5, 2017

azasypkin Dec 4, 2017

spalger Dec 4, 2017

azasypkin Dec 4, 2017

azasypkin Dec 4, 2017

azasypkin Dec 4, 2017

spalger Dec 4, 2017 •

edited

Loading

azasypkin Dec 4, 2017

spalger Dec 4, 2017

azasypkin Dec 4, 2017

kimjoar left a comment

spalger commented Dec 6, 2017

[CI] Produce junit test reports #15281

[CI] Produce junit test reports #15281

Conversation

spalger commented Nov 30, 2017 • edited Loading

azasypkin left a 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

spalger Dec 4, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Dec 4, 2017 • edited Loading

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

kimjoar left a comment

Choose a reason for hiding this comment

spalger commented Dec 6, 2017

spalger commented Nov 30, 2017 •

edited

Loading

spalger Dec 4, 2017 •

edited

Loading

spalger Dec 4, 2017 •

edited

Loading