Improve jest mocking infrastructure #38760

timroes · 2019-06-12T07:44:56Z

Summary

This PR improves and aligns the jest mock infrastructure in the following way:

Make sure __mocks__ files (jest mocks) are no longer bundled inside the Karma/Mocha test bundles.
Make sure ESLint knows that jest is the environment in __mocks__ files.
Don't have a high-level __mocks__ folder in x-pack/plugins anymore that mocks away modules from ui/public.

x-pack should not provide any top level __mocks__ for ui/public modules anymore. Mocks should either live locally inside test suites (or extracted "locally" into files, like I did in the spaces plugin). Generic mocks should live inside a __mocks__ folder beside their implementation. Everyone can then simply enabling those mocks by calling e.g. jest.mock('ui/kfetch') and Jest will automatically find the appropriate mock file beside the implementation file.

We also enable mocks for a couple of very very basic modules by default in all Jest tests that would otherwise fail a lot of test. These modules are currently: ui/chrome and ui/metadata.

You might now see an error message like:

jest-haste-map: duplicate manual mock found:
  Module name: index
  Duplicate Mock path: /home/timroes/elastic/kibanaPink/src/legacy/ui/public/kfetch/__mocks__/index.ts
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in: 
/home/timroes/elastic/kibanaPink/src/legacy/ui/public/kfetch/__mocks__/index.ts
 Please delete one of the following two files: 
 /home/timroes/elastic/kibanaPink/src/legacy/ui/public/chrome/__mocks__/index.js
/home/timroes/elastic/kibanaPink/src/legacy/ui/public/kfetch/__mocks__/index.ts

when running jest tests. This is a jest bug, that's tracked in jestjs/jest#2070

I mainly touched two test suites and moved mocks into those, so I would appreciate a quick glance by the @elastic/kibana-security team and the @elastic/secops team for their test suites.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-06-12T07:44:58Z

Pinging @elastic/kibana-operations

elasticmachine · 2019-06-12T08:46:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mistic · 2019-06-12T11:45:09Z

I think having the input from @elastic/kibana-platform team here would be a good idea, but in my opinion I think that instead of auto mocking things we should abstract the mock in a file and then import it where we need it. That way is easier to understand what is going on the tests without affecting the entire test sctructure.

timroes · 2019-06-12T12:29:56Z

@mistic I synced with Spencer a lot about that issue, and for now our idea was that we're mocking away the very fundamental modules everyone would need to mock (chrome, metadata and createLegacyClass). The problem is, that you nearly never use those modules at all, but because they are used somewhere down the import tree by something you depend you have to mock them. I don't think it's a) increasing test readability and b) clean code if we require (nearly) every test to mock out those things, even though they don't use them, or even have an idea where they might be used (besides looking at the stack trace of course). That way when writing test we force users to spend more time into actually understanding what the code does they require and how to mock away code for that, then actually testing THEIR code.

Those provided mocks themselves should nevertheless be very stupid and not have any logic (so not how the chrome mock in x-pack currently has). Instead if you need that specific logic you can just import those modules in your tests and modify the mock implementation.

Also we currently have unnecessarily different mocks for x-pack than for OSS, which we should neither have ideally, but have shared mocks (the same as more shared configs in the long run).

I am happy to take that into a larger round of discussion, but since I know this is at the moment blocking at least 2 other PRs I really want to proceed fast on this.

mistic · 2019-06-12T16:41:49Z

Yeah after a small meeting about that it was more clear the problems that route us to that possible solution. While I think we could manually import predefined mocks for chrome and metadata where we need it because that would simplify problems, I think we can also consider to auto mock those specific dependencies in case that help us somehow.

elasticmachine · 2019-06-12T17:14:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2019-06-12T17:46:15Z

Jenkins, test this - CI outage so I cancelled

timroes · 2019-06-12T18:02:00Z

packages/eslint-config-kibana/jest.js

@@ -3,6 +3,7 @@ module.exports = {
    {
      files: [
        '**/*.{test,test.mocks,mock}.{js,ts,tsx}',
+        '**/__mocks__/**/*.{js,ts,tsx}',


ℹ️ This way we make eslint aware of jest in all __mocks__ directories.

timroes · 2019-06-12T18:03:24Z

src/legacy/core_plugins/tests_bundle/find_source_files.js

@@ -36,6 +36,7 @@ const findSourceFiles = async (patterns, cwd = fromRoot('.')) => {
        '**/_*.js',
        '**/*.test.js',
        '**/*.test.mocks.js',
+        '**/__mocks__/**/*',


ℹ️ Without that line __mocks__ files (which could contain calls to jest) would be bundled for Karma/Mocha tests and then cause them to fail, since jest is not available there.

elasticmachine · 2019-06-12T19:22:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-13T13:42:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

rudolf · 2019-06-14T09:22:18Z

I like the simplicity of automatically loading the correct mock with jest.mock('ui/foobar') 👍

Auto-mocking things like ui/metadata will make testing easier for a developer, but I wonder if it isn't hiding code smells. Anytime you unit test something and realise your tests fail because an unrelated module wasn't mocked you have to ask yourself what's wrong here. When it's automatically mocked you live with the impression that your unit under test is actually isolated when it isn't.

I don't know if this is true in every case, but taking LanguageSwitcher as an example, I believe kueryQuerySyntaxDocs should be passed in as a prop so that this react component can be completely dumb. Receiving all of it's data as props it won't need a metadata mock. Then when it comes to integration testing it'll be easy to see which dependencies need to be mocked as these aren't spread out through the components of a UI. Hiding the need for a mock hides that this component isn't following the react paradigm of all data as props (or context).

I can imagine that our legacy angular code hides many dependencies in subtle ways making refactoring like this to fix the problem hard. But perhaps having to sprinkle jest.mock('ui/metadata') all over our tests is a good reminder and a way for us to pay for our past sins 😹

timroes · 2019-06-14T09:48:26Z

@skaapgif I partly agree with you, why the suggestion was only to automatically enable absolute fundamental mocks. Also after discussing with Spencer yesterday we agreed to remove the auto mocking of ui/kfetch again. So it would really only be ui/metadata and ui/chrome for now. I still think it would be fine auto mocking those, since I still think we should not force to know someone writing tests for e.g. the rollup management UI, to have knowledge about that something requires ui/metadata to be mocked (something that's totally unrelated to them, in that case it would be rollup view -> some static utility from the data plugin -> data plugin -> interpreter -> ... 5 dependencies later ... -> doc_links -> metadata).

Even given our past sins, I am not sure if it's a good thing to force that onto every (unrelated) user, maybe even that user that wrote their code 100% perfect and mockable and so on. I would be (and thus the very long description in the file) very very careful what to automock and always have this close to nothing. But ui/metadata is a file that's usually filled with content within the build process and just isn't that during tests, so I think it's valid to give this also a "working in tests" implementation (to be fair an alternative could be, simply making that build process step working in jest tests).

rudolf · 2019-06-14T12:24:03Z

Yeah I agree that there's a balance between "paying for your sins" and every developer being punished by bad practises they have no control over.

spalger

One tiny change, otherwise LGTM

spalger · 2019-06-14T13:02:42Z

src/dev/jest/setup/mocks.js

+ * The mocks that are enabled that way live inside the `__mocks__` folders beside their implementation files.
+ */
+
+jest.mock('ui/kfetch');


Let’s remove this, kfetch feels like something that should be under control of the units being tested.

elasticmachine · 2019-06-14T17:03:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2019-06-14T20:03:49Z

Jenkins, Test this - failure to download ES

spalger

LGTM on green

elasticmachine · 2019-06-14T20:28:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2019-06-14T20:30:29Z

Jenkins, test this - artifact download error again :(

elasticmachine · 2019-06-14T21:35:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Always mock metadata/chrome in OSS * Enable jest env in jest mocks * Exclude jest mocks in karma bundles * Use setupFilesAfterEnv in config * Remove chrome/metadata mock from x-pack * Remove kuery mock * Add missing mock to SIEM test * Fix typo in mock import * Remove top level capabilities x-pack mock * Move kfetch mock to ui/public * Move moment-timezone to mocks file * Unmock kfetch in kfetch specific tests * Make kfetch mock manual * Removed unnecessary jest.mock * Remove kfetch unmocks

Always mock metadata/chrome in OSS

2df5647

timroes added chore Team:Operations Team label for Operations Team labels Jun 12, 2019

timroes added 2 commits June 12, 2019 09:45

Enable jest env in jest mocks

a92e47e

Exclude jest mocks in karma bundles

aa2e170

timroes changed the title ~~Always mock metadata/chrome in OSS~~ Improve jest mocking infrastructure Jun 12, 2019

mshustov added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Jun 12, 2019

Use setupFilesAfterEnv in config

aea76ca

timroes added 3 commits June 12, 2019 19:26

Remove chrome/metadata mock from x-pack

45b8ac2

Remove kuery mock

0e17aed

Add missing mock to SIEM test

e9a72e8

This comment has been minimized.

Sign in to view

timroes commented Jun 12, 2019

View reviewed changes

Fix typo in mock import

c338d16

timroes added 2 commits June 13, 2019 13:15

Remove top level capabilities x-pack mock

b31ab73

Move kfetch mock to ui/public

c586703

timroes added v7.4.0 v8.0.0 v7.3.0 and removed v7.4.0 labels Jun 13, 2019

Move moment-timezone to mocks file

c32f0bd

timroes requested a review from a team as a code owner June 13, 2019 12:30

timroes added 2 commits June 13, 2019 14:32

Merge branch 'master' into jest-mocks

27e6a4f

Unmock kfetch in kfetch specific tests

e6c0497

kobelb approved these changes Jun 13, 2019

View reviewed changes

timroes added the release_note:skip Skip the PR/issue when compiling release notes label Jun 14, 2019

spalger suggested changes Jun 14, 2019

View reviewed changes

timroes added 2 commits June 14, 2019 17:46

Make kfetch mock manual

101c749

Merge branch 'master' into jest-mocks

36e0c41

timroes requested review from a team as code owners June 14, 2019 16:01

Removed unnecessary jest.mock

693908c

timroes removed the request for review from a team June 14, 2019 16:04

Remove kfetch unmocks

5cc2338

spalger approved these changes Jun 14, 2019

View reviewed changes

Merge branch 'master' into jest-mocks

4c77c9a

timroes merged commit 2d37b05 into elastic:master Jun 14, 2019

timroes deleted the jest-mocks branch June 14, 2019 21:36

timroes mentioned this pull request Jun 14, 2019

[7.x] Improve jest mocking infrastructure (#38760) #39033

Merged

timroes mentioned this pull request Aug 17, 2020

Jest duplicate mocks names warning #75135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve jest mocking infrastructure #38760

Improve jest mocking infrastructure #38760

timroes commented Jun 12, 2019 •

edited

Loading

elasticmachine commented Jun 12, 2019

elasticmachine commented Jun 12, 2019

mistic commented Jun 12, 2019

timroes commented Jun 12, 2019

mistic commented Jun 12, 2019

elasticmachine commented Jun 12, 2019

This comment has been minimized.

timroes commented Jun 12, 2019

timroes Jun 12, 2019 •

edited

Loading

timroes Jun 12, 2019

elasticmachine commented Jun 12, 2019

elasticmachine commented Jun 13, 2019

rudolf commented Jun 14, 2019

timroes commented Jun 14, 2019

rudolf commented Jun 14, 2019

spalger left a comment •

edited

Loading

spalger Jun 14, 2019

elasticmachine commented Jun 14, 2019

timroes commented Jun 14, 2019

spalger left a comment

elasticmachine commented Jun 14, 2019

timroes commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

Improve jest mocking infrastructure #38760

Improve jest mocking infrastructure #38760

Conversation

timroes commented Jun 12, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jun 12, 2019

elasticmachine commented Jun 12, 2019

💚 Build Succeeded

mistic commented Jun 12, 2019

timroes commented Jun 12, 2019

mistic commented Jun 12, 2019

elasticmachine commented Jun 12, 2019

💚 Build Succeeded

This comment has been minimized.

timroes commented Jun 12, 2019

timroes Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

timroes Jun 12, 2019

Choose a reason for hiding this comment

elasticmachine commented Jun 12, 2019

💚 Build Succeeded

elasticmachine commented Jun 13, 2019

💚 Build Succeeded

rudolf commented Jun 14, 2019

timroes commented Jun 14, 2019

rudolf commented Jun 14, 2019

spalger left a comment • edited Loading

Choose a reason for hiding this comment

spalger Jun 14, 2019

Choose a reason for hiding this comment

elasticmachine commented Jun 14, 2019

💔 Build Failed

timroes commented Jun 14, 2019

spalger left a comment

Choose a reason for hiding this comment

elasticmachine commented Jun 14, 2019

💔 Build Failed

timroes commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

💚 Build Succeeded

timroes commented Jun 12, 2019 •

edited

Loading

timroes Jun 12, 2019 •

edited

Loading

spalger left a comment •

edited

Loading