[Enterprise Search] Jest config & handy Jest script #84839

cee-chen · 2020-12-02T22:01:46Z

Summary

Updates our new Jest config file to automatically report coverage (no more copying and pasting super long CLI commands!)
Because I'm very extra, I also added a shell script that makes the dev experience of running tests (& getting coverage) on a specific component folder easier. Example API:

# Run commands from the `enterprise_search` plugin folder root

# run and get coverage on all tests
sh jest.sh

# run and get coverage on specific folder
sh jest.sh public/applications/shared/http

Let me know what you think y'all!! 🙇‍♀️

Checklist

Documentation was added for features that require explanation or tutorials

scottybollinger

This is awesome! Thanks for making our lives easier!

🎉 🎉 🎉

tylersmalley · 2020-12-02T23:28:34Z

Just a heads up, I am working on a global runner which I believe will solve some of the things you're doing here. My WIP is here: #84848

But essentially it will update yarn test:jest to find the right config file. You can either run it in the a folder within your plugin, or pass a path to your plugin to it.

cee-chen · 2020-12-02T23:51:04Z

Oooh!! Thanks so much Tyler! 🎉 I can definitely hang tight on this PR until yours is merged in and play around with that.

You can either run it in the a folder within your plugin,

Fantastic, this would be amazing! I've had some issues recently with collectCoverageFrom not really working as expected (it doesn't seem to listen to the roots setting, but seems to always require explicitly pathing from rootDir) and I'm hoping your PR might solve that. If I can get around not needing the jest.sh script I'll definitely be a happy camper.

tylersmalley · 2020-12-03T00:48:04Z

Sounds great! Yeah, it should remove the need for the jest.sh file entirely, with the bonus it's able to be used across the repository. I am not sure abou the collectCoverageFrom issue you're referring to. Can you provide some reproduction details, or want to get on a quick Zoom to go over it?

cee-chen · 2020-12-16T01:19:16Z

@tylersmalley The command works fantastic for only running tests within the current folder/subdir, but unfortunately I couldn't get the new yarn test:jest command quite working the way I wanted w/ collection coverage. The end result I'm after is running a single command and having it only report coverage for that folder, e.g.:

Unfortunately current behavior for running yarn test:jest --coverage from within subfolders still reports coverage for everything in the entire enterprise_search plugin, not just the current folder. yarn test:jest --collectCoverageFrom=./ doesn't seem to work either (I'm guessing because --collectCoverageFrom requires rootDir).

I opted to keep using the jest.sh script, because using it from the enterprise_search plugin dir allows tab to autocomplete folder names which is always nice. LMK if you see any issues or workarounds at all!

- No more super intricate yarn commands! Hooray

…rs/subdirectories

kibanamachine · 2020-12-16T18:41:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6e947d8

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	47280	48041	+761

History

💚 Build #94784 succeeded 93c663116ad44fdaf7606cdeee2fb9c8189ee10b
💚 Build #91542 succeeded b747758a7f29e5ecff50306301d8d54c01fb09b0

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

tylersmalley · 2020-12-16T21:44:47Z

x-pack/plugins/enterprise_search/jest.config.js

@@ -8,4 +8,11 @@ module.exports = {
  preset: '@kbn/test',
  rootDir: '../../..',
  roots: ['<rootDir>/x-pack/plugins/enterprise_search'],
+  collectCoverage: true,


I don't think you want to enable coverage by default. In your jest.sh file you can pass --coveage to do this.

Hm, from a team perspective I'd prefer it for this project - coverage is fairly important to us and something I want our front end devs to always have in front of them.

If this has implications for CI or affects processes outside just our team/code though I can definitely turn it off and only set it in jest.sh - if not, I'd prefer to leave it as the default

I appreciate the importance of code coverage. While it won't have an impact on CI it does impact the default experience of anyone running yarn jest:test within the plugin. It will by default include code coverage for the entire plugin which requires a lot of scrolling to get to the normal output.

I will note that we are working on having code coverage reports/metrics as part of the pull request details.

While it won't have an impact on CI it does impact the default experience of anyone running yarn jest:test within the plugin

This should mostly be just Enterprise Search frontend devs though, right? In which case our team would fully expect this behavior. Is the concern for other non-Enterprise Search devs who need to make changes to our files/repo? (e.g., Kibana ops, etc.) That's definitely valid if so, but I would argue that it's a minor nuisance at worst for non-Enterprise Search devs who rarely need to make changes / run tests directly on our repo, but it's a major quality of life default for Enterprise Search devs since we're working on our code constantly and essentially always want to see accompanying coverage %s when we're running tests.

I will note that we are working on having code coverage reports/metrics as part of the pull request details.

Oh wow, that's amazing! I love developing on the Kibana platform so much, y'all just continue to be blow me away 🎉 Just curious, will the code coverage report for the touched files in the PR only, or reported based on the nearest jest config file (i.e. the entire plugin)?

This should mostly be just Enterprise Search frontend devs though, right? In which case our team would fully expect this behavior. Is the concern for other non-Enterprise Search devs who need to make changes to our files/repo? (e.g., Kibana ops, etc.) That's definitely valid if so, but I would argue that it's a minor nuisance at worst for non-Enterprise Search devs who rarely need to make changes / run tests directly on our repo, but it's a major quality of life default for Enterprise Search devs since we're working on our code constantly and essentially always want to see accompanying coverage %s when we're running tests.

Sounds like you have thought it through. I was just raising to make sure you're aware and sounds like this is what you expect. 👍

Oh wow, that's amazing! I love developing on the Kibana platform so much, y'all just continue to be blow me away tada Just curious, will the code coverage report for the touched files in the PR only, or reported based on the nearest jest config file (i.e. the entire plugin)?

It will be for the touched files. I really feel that is where the value of a code coverage report comes in, and it must be available at review time. The first step of this is to include the change as part of the metrics. However, we intend to iterate on this over time.

It will be for the touched files. I really feel that is where the value of a code coverage report comes in, and it must be available at review time.

Love it, that's totally what I was hoping for! Can't wait to see it happen, I'll definitely owe you some beers at the next all hands (whenever that is, haha 🍻 )

95% of the time that I'm running jest it's with the --watch flag, in which case, I do not need coverage reports, and probably don't want them to be run if it slows down the speed for tests. Do you know what effect this has on that use case?

@JasonStoltz With --watch on, it reports coverage for only the file that you touched:

That was on a server ts file, when I made changes on a Kea logic file and on a component tsx file, I saw some different/odd behavior (all 0s) but still nothing (IMO) that would significantly affect speed since coverage was still only being reported for 1 file vs the entire plugin.

I'm going to go ahead and merge this plugin in now as a heads up since it's been open a while - if you see significant performance issues due to text coverage reports, definitely let me know and we can continue to adjust.

Ah, well that is actually really rad if it only reports coverage on the file you touched. That's a big plus. I was seeing different behavior when I tried it out.

I think it's different now with Tyler's changes + (perhaps?) the jest config changes in this PR - definitely feel free to ping if you you see otherwise locally on latest master!

tylersmalley · 2020-12-16T21:45:34Z

x-pack/plugins/enterprise_search/jest.config.js

@@ -8,4 +8,11 @@ module.exports = {
  preset: '@kbn/test',
  rootDir: '../../..',
  roots: ['<rootDir>/x-pack/plugins/enterprise_search'],
+  collectCoverage: true,
+  coverageReporters: ['text'],
+  collectCoverageFrom: [


Passing this through the CLI will overwrite this setting.

Right yeah, overriding this setting via CLI is intentional for reporting specific folder coverage. Do you see that as being an issue?

I just wanted to point it out to make sure it's not being set if it's not intentionally used. Setting it here does deviate from the defaults of the preset defined here.

Oooh, nice - I really like those presets, I wonder if there's any easy way to merge those in here 🤔 This collectCoverageFrom is being intentionally used here, essentially what I wanted was to to skip reporting for our public/plugin.ts and server/plugin.ts files (AFAIK there's no real meaningful way to write unit tests for those files, correct me if I'm wrong).

tylersmalley · 2020-12-16T21:46:42Z

@constancecchen, thanks for the clairification. I believe this is something we could add to our wrapper. It does get a bit complicated as we will need to merge in the coverage settings from the project config or the jest-preset but it could be done. Mind creating an issue for it so we can track?

cee-chen · 2020-12-16T21:59:37Z

Yes, no problem at all! Here it is: #86199

No rush on it as well, I'm personally super happy with the existing yarn test:jest and using a shortcut/script file for now.

* Update Jest config to automatically report coverage - No more super intricate yarn commands! Hooray * Add handy shell script for running tests & coverage on specific folders/subdirectories

cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 2, 2020

cee-chen requested a review from a team December 2, 2020 22:01

scottybollinger approved these changes Dec 2, 2020

View reviewed changes

cee-chen marked this pull request as draft December 3, 2020 23:05

cee-chen force-pushed the jest-config branch from b747758 to e59cc67 Compare December 16, 2020 00:32

cee-chen marked this pull request as ready for review December 16, 2020 00:33

cee-chen force-pushed the jest-config branch 2 times, most recently from 17c6fe2 to 93c6631 Compare December 16, 2020 01:13

cee-chen added 2 commits December 16, 2020 08:29

Update Jest config to automatically report coverage

db736c1

- No more super intricate yarn commands! Hooray

Add handy shell script for running tests & coverage on specific folde…

6e947d8

…rs/subdirectories

cee-chen force-pushed the jest-config branch from 93c6631 to 6e947d8 Compare December 16, 2020 16:30

tylersmalley reviewed Dec 16, 2020

View reviewed changes

cee-chen mentioned this pull request Dec 16, 2020

[Jest][request] When running yarn test:jest in a specific folder, only collect coverage within that folder #86199

Open

cee-chen merged commit 694bbd4 into elastic:master Dec 17, 2020

cee-chen deleted the jest-config branch December 17, 2020 16:19

cee-chen added v7.12.0 and removed v7.11.0 labels Dec 17, 2020

cee-chen mentioned this pull request Dec 17, 2020

[7.x] [Enterprise Search] Jest config & handy Jest script (#84839) #86317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enterprise Search] Jest config & handy Jest script #84839

[Enterprise Search] Jest config & handy Jest script #84839

cee-chen commented Dec 2, 2020 •

edited

Loading

scottybollinger left a comment

tylersmalley commented Dec 2, 2020

cee-chen commented Dec 2, 2020

tylersmalley commented Dec 3, 2020

cee-chen commented Dec 16, 2020

kibanamachine commented Dec 16, 2020

tylersmalley Dec 16, 2020

cee-chen Dec 16, 2020

tylersmalley Dec 16, 2020

cee-chen Dec 16, 2020

tylersmalley Dec 16, 2020

cee-chen Dec 16, 2020

JasonStoltz Dec 17, 2020

cee-chen Dec 17, 2020

JasonStoltz Dec 17, 2020

cee-chen Dec 17, 2020

tylersmalley Dec 16, 2020

cee-chen Dec 16, 2020

tylersmalley Dec 16, 2020

cee-chen Dec 16, 2020

tylersmalley commented Dec 16, 2020

cee-chen commented Dec 16, 2020

[Enterprise Search] Jest config & handy Jest script #84839

[Enterprise Search] Jest config & handy Jest script #84839

Conversation

cee-chen commented Dec 2, 2020 • edited Loading

Summary

Checklist

scottybollinger left a comment

Choose a reason for hiding this comment

tylersmalley commented Dec 2, 2020

cee-chen commented Dec 2, 2020

tylersmalley commented Dec 3, 2020

cee-chen commented Dec 16, 2020

kibanamachine commented Dec 16, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

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

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

tylersmalley commented Dec 16, 2020

cee-chen commented Dec 16, 2020

cee-chen commented Dec 2, 2020 •

edited

Loading