Skip to content
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

Fixes #29637 - Allow jest testing to use foremanReact #8682

Merged
merged 1 commit into from
May 4, 2020

Conversation

johnpmitsch
Copy link
Contributor

@johnpmitsch johnpmitsch commented Apr 28, 2020

We are currently mocking foremanReact, which is the alias for Foreman's React components and javascript library, while testing. This has been working fine, but is becoming more of an issue as we use and move more logic to Foreman's React components, meaning we have to mock a lot of logic that is used in our components which can limit our testing ability.

Jest (the React test runner) allows local modules to be specified by path, which allows us to use foremanReact as long as foreman is present in a sibling directory to Katello.

This will allow us to use the foremanReact module as-is, which is helpful now but also will be necessary when major components such as tables are moved to Foreman and we will need a way to use them in testing.

The moduleNameMapper is what allows us to do this. The jest config is moved to jest.config.js so nodejs's path module can be used to dynamically determine the path.

.babelrc is moved to babel.config.js as the foremanReact files served directly from foreman were not transpiling during testing. There is a comment on a jest issue that explains the reasoning for this. My understanding is that jest will look up the tree for .babelrc rather than babel.config.js, which allows multi-repo setups to use transpiled files outside of the repo.

I didn't remove some of the mocks because either they created a lot of deprecation/console noise during the tests or they didn't provide consistent snapshots. I think its fine to keep some existing mocks of foremanReact components as the point is to allow the usage of foremanReact directly. It's also worth noting that parts of foremanReact can still be mocked as they have been.

@theforeman-bot
Copy link

Issues: #29637

@johnpmitsch
Copy link
Contributor Author

@johnpmitsch
Copy link
Contributor Author

@ehelms @ekohl In Jenkins CI, is the foreman directory checked out in the same directory as Katello? For example, at /home/jenkins/workspace/foreman.

@ekohl
Copy link
Member

ekohl commented Apr 28, 2020

https://ci.theforeman.org/blue/organizations/jenkins/katello-pr-test/detail/katello-pr-test/3610/pipeline/12 has logs:

[2020-04-28T00:38:33.778Z] Cloning repository https://github.com/Katello/katello
[2020-04-28T00:38:33.779Z]  > git init /home/jenkins/workspace/katello-pr-test # timeout=10

Then

[2020-04-28T00:38:47.505Z] Cloning repository https://github.com/theforeman/foreman
[2020-04-28T00:38:47.506Z]  > git init /home/jenkins/workspace/katello-pr-test/foreman # timeout=10

@johnpmitsch johnpmitsch changed the title Fixes #29637 - Allow jest testing to use foremanReact [DO NOT MERGE] Fixes #29637 - Allow jest testing to use foremanReact Apr 28, 2020
@johnpmitsch
Copy link
Contributor Author

https://ci.theforeman.org/blue/organizations/jenkins/katello-pr-test/detail/katello-pr-test/3610/pipeline/12 has logs:

[2020-04-28T00:38:33.778Z] Cloning repository https://github.com/Katello/katello
[2020-04-28T00:38:33.779Z]  > git init /home/jenkins/workspace/katello-pr-test # timeout=10

Then

[2020-04-28T00:38:47.505Z] Cloning repository https://github.com/theforeman/foreman
[2020-04-28T00:38:47.506Z]  > git init /home/jenkins/workspace/katello-pr-test/foreman # timeout=10

Thanks! I now see that foreman is checked out in Katello itself.

I'm testing out hardcoing the jenkins path just to prove that this will work as expected. For this to work across environments, we will have to have some sort of logic to determine where foreman is, either through an environment variable or checking that it is a child directory of Katello first.

@ekohl
Copy link
Member

ekohl commented Apr 28, 2020

You can convert it to draft (top right, in the review block) if it shouldn't be merged.

@johnpmitsch johnpmitsch marked this pull request as draft April 28, 2020 16:41
@johnpmitsch johnpmitsch changed the title [DO NOT MERGE] Fixes #29637 - Allow jest testing to use foremanReact Fixes #29637 - Allow jest testing to use foremanReact Apr 28, 2020
@johnpmitsch johnpmitsch force-pushed the foremanReactTesting branch 9 times, most recently from ccda76a to 9eca1fa Compare April 30, 2020 20:15
@johnpmitsch
Copy link
Contributor Author

Updated to dynamically determine where foreman is when testing, checking sibling, child, and parent directories. This allows foremanReact to be used both locally and in a CI environment by jest.

I also added a mock for react-intl as that is provided by foreman and not present when only katello npm packages are installed. We could also duplicate the react-intl packages in the devDependencies section of our package.json, which we do for some other packages, but the mock is pretty simple and prevents the need for the duplication.

Here is an example error when foreman directory isn't present:

Error: Foreman directory can not be found! These tests require Foreman to be present in either a parent, sibling, child directory relative to Katello and contain the expected files in ./webpack/assets/javascripts/react_app, relative to the Foreman directory.
    at Object.<anonymous> (/home/jomitsch/misc/katello/jest.config.js:25:30)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at _default (/home/jomitsch/misc/katello/node_modules/jest-config/build/readConfigFileAndSetRootDir.js:51:20)
    at readConfig (/home/jomitsch/misc/katello/node_modules/jest-config/build/index.js:160:59)

@johnpmitsch johnpmitsch marked this pull request as ready for review April 30, 2020 20:31
@johnpmitsch johnpmitsch force-pushed the foremanReactTesting branch 2 times, most recently from 99356be to 93bb8eb Compare April 30, 2020 23:26
Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @johnpmitsch! Web UI and tests all run and pass. Now that the PR is here, I see it's much easier than I thought it would be. We should have done this a long time ago.

I have no philosophical issue with not mocking foremanReact, because in a "normal" app without plugin architecture, you wouldn't have to mock this sort of thing anyway. So, ACK besides the one grammar nitpick below.

jest.config.js Outdated

const foremanReactRelative = 'webpack/assets/javascripts/react_app';
const possibleForemanLocations = ['./foreman', '../foreman', '../../foreman'];
const notFound = 'Foreman directory can not be found! These tests require Foreman to be present ' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const notFound = 'Foreman directory can not be found! These tests require Foreman to be present ' +
const notFound = 'Foreman directory cannot be found! These tests require Foreman to be present ' +

const foremanReactRelative = 'webpack/assets/javascripts/react_app';
const possibleForemanLocations = ['./foreman', '../foreman', '../../foreman'];
const notFound = 'Foreman directory can not be found! These tests require Foreman to be present ' +
'in either a parent, sibling, or child directory relative to Katello and contain the expected ' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was about to ask who in their right mind would put the foreman directory inside of Katello, but then saw the other PR comments 😆

Jest allows local modules to be specified by path, which allows us to use foremanReact as long as foreman is present in a sibling directory to Katello
Copy link
Member

@jturel jturel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great change. Thanks for taking it on ... it really didn't feel right to be mocking out all those modules.

@johnpmitsch johnpmitsch merged commit cb8ed4d into Katello:master May 4, 2020
@johnpmitsch johnpmitsch deleted the foremanReactTesting branch May 4, 2020 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants