-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Code coverage setup on CI #49003
Code coverage setup on CI #49003
Conversation
💚 Build Succeeded
|
💔 Build Failed
|
@elasticmachine merge upstream |
💚 Build Succeeded
|
/cc @wayneseymour @LeeDr |
💚 Build Succeeded
|
💚 Build Succeeded
|
Squash to single commmit, cherry pick this guy ontop of something historical, Like a week prior or so, ....keep iterating over like 10 weeks so we can visualize. run the reports on like 5 big gce instances to create the reports |
💚 Build Succeeded
|
💚 Build Succeeded
|
💚 Build Succeeded
|
💔 Build Failed
|
@elasticmachine merge upstream |
💚 Build Succeeded
|
💚 Build Succeeded
|
💔 Build Failed
|
💔 Build Failed
|
💔 Build Failed
|
17b6626
to
6c09761
Compare
💔 Build Failed
|
…to code-coverage-on-ci
@elasticmachine merge upstream |
@dmlemeshko is there no issue with all of the coverage archives having the same name? I noticed it in the image you posted. |
'xpack-ciGroup10': kibanaPipeline.getXpackCiGroupWorker(10), | ||
]), | ||
]) | ||
kibanaPipeline.jobRunner('tests-l', false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmlemeshko 'tests-l'
==== some large instance for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the default one (PRs) has not enough space for coverage processing (25+ GB)
@@ -193,7 +193,7 @@ export default () => | |||
.default('localhost'), | |||
watchPrebuild: Joi.boolean().default(false), | |||
watchProxyTimeout: Joi.number().default(10 * 60000), | |||
useBundleCache: Joi.boolean().default(Joi.ref('$prod')), | |||
useBundleCache: Joi.boolean().default(!!process.env.CODE_COVERAGE ? true : Joi.ref('$prod')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that the schema is the best place to put logic like this...
Actually, do you need to set this at all? It's always set to true here:
Line 41 in 08537b6
buildArgs: ['--optimize.useBundleCache=true'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is somehow without setting useBundleCache
in a schema, I'm getting flaky tests and maybe it is because we don't make a build, but run tests from the source. This change was also made in discussion with @spalger. If I remove it, tests are started to fail for code coverage runs.
test/scripts/jenkins_ci_group.sh
Outdated
yarn run grunt run:pluginFunctionalTestsRelease --from=source; | ||
yarn run grunt run:exampleFunctionalTestsRelease --from=source; | ||
yarn run grunt run:interpreterFunctionalTestsRelease; | ||
checks-reporter-with-killswitch " Functional tests with code coverage / Group ${CI_GROUP}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need GitHub checks support? If not, you can just replace these two lines with just yarn run grunt "run:functionalTests_ciGroup${CI_GROUP}";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for the other spots where you have checks-reporter-with-killswitch
test/scripts/jenkins_xpack.sh
Outdated
node ./legacy/plugins/canvas/scripts/shareable_runtime | ||
checks-reporter-with-killswitch "X-Pack Jest Coverage" node scripts/jest --ci --verbose --coverage | ||
# rename file in order to be unique one | ||
mv ../target/kibana-coverage/jest/coverage-final.json ../target/kibana-coverage/jest/xpack-coverage-final.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a test fails in the above command, will the file still get output? If so, it won't get renamed, because the script will exit when the test fails
.ci/Jenkinsfile_coverage
Outdated
kibanaPipeline.jobRunner('tests-l', false) { | ||
kibanaPipeline.downloadCoverageArtifacts() | ||
kibanaPipeline.bash( | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually put scripts that get this long into their own files, e.g. in test/scripts
or in .ci
... Not required though. You could also change it to '''
instead of """
and you wouldn't need to worry about escaping the $
s (since you aren't using interpolation)
@@ -17,12 +17,23 @@ | |||
* under the License. | |||
*/ | |||
|
|||
require('../src/setup_node_env'); | |||
require('@kbn/test').runTestsCli([ | |||
// eslint-disable-next-line no-restricted-syntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would all of these changes feel better as a separate file (e.g. scripts/functional_tests_for_coverage.js
or similar)? You'd be able to skip the list combining and eslint disables... I can't decide, just a thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in a separate file initially, but I talked to Spencer and made it this way. @spalger is ok or better in an own file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, but Brian's comments make sense.
node ./legacy/plugins/canvas/scripts/shareable_runtime | ||
node scripts/jest --ci --verbose --coverage | ||
# rename file in order to be unique one | ||
test -f ../target/kibana-coverage/jest/coverage-final.json \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test
will fail the build if the file isn't there (because it returns a non-zero exit code), in case that isn't what you want.
Also, you still might have the problem I mentioned before:
If node scripts/jest --ci --verbose --coverage
has a test that fails, this script will exit before the rename happens. So, later on, when you expect the file to be called xpack-coverage-final.json
, it will be called coverage-final.json
. Is this a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if any test will fail coverage-final.json
won't be generated. And actually we agreed not to proceed with coverage merging if any tests failed.
So exiting before renaming is fine, build should be red and next step (merge) is not starting. If the tests pass, json file is expected to be generated and I rename it because the json with the same name will be generated for oss jest tests.
During merge step I extract both oss and xpack in the same folder for merging, names should be unique to avoid overwriting.
What do you think is the best solution with context I provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just asked @LeeDr and he feels we should not push the coverage data if there's a failure as that will skew the coverage numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, just wanted to make sure the file not getting renamed wasn't going to cause a downstream issue!
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* master: (23 commits) [Vis: Default editor] Reactify the timelion editor (elastic#52990) [Discover] fix histogram min interval (elastic#53979) [Telemetry] [Monitoring] Only retry fetching usage once monito… (elastic#54309) [docs][APM] Add runtime index config documentation (elastic#53907) [SIEM] Detection engine timeline (elastic#53783) Filter scripted fields preview field list to source fields (elastic#53826) Management - New platform api (elastic#52579) Reset region and Account when switching inventory (elastic#54287) [SIEM] [Case] Case workflow api schema (elastic#51535) Code coverage setup on CI (elastic#49003) [ML] DF Analytics Results: adds link to docs (elastic#54189) Update schemas boolean, byteSize, and duration to coerce strings (elastic#54177) [Metrics UI] Pass relevant shouldAllowEdit capabilities into SettingsPage (elastic#49781) [Canvas] Fixes bugs with autoplay and refresh (elastic#53149) [ML] DF Analytics Classification: ensure confusion matrix can be fetched (elastic#53629) Fix Vega react eslint errors (elastic#54259) Remove non existing codeowners (elastic#54274) use correct type (elastic#54244) [Dashboard] Removing 100% as dshDashboardViewport height (elastic#54263) add `examples/` to no-restricted-path config (elastic#54252) ...
linking #22221 |
Summary
Integrating code coverage (currently mocha, jest, and functional tests) into our CI
Currently running on Jenkins: kibana-code-coverage job
Details
Build and tests execution part:
--optimize.useBundleCache=true
to reuse cache if it's available and avoid concurrent source modification. However, x-pack ciGroups have different configs and it will force to reoptimize. To solve the issue, x-pack functional tests are executed on 3 workers, not 1.kibana-coverage.tar.gz
and upload to GCS along with usual tests artifacts.Code coverage merging part:
Combined coverage for mocha, jest and functional tests is provided in 2 formats:
json-summary
, to push data into build stats ES and monitor coverage within the time periodhtml
, to publish as a static web-site on GCS and have quick access