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

Normalize config and remove unecessary checks #7801

Merged
merged 7 commits into from Feb 18, 2019

Conversation

@lekterable
Copy link
Contributor

lekterable commented Feb 4, 2019

Summary

shouldInstrument function expects the config object to be normalized, but some of the unit tests don't respect that so I've tried to change this. When we will be sure that the received config is normalized we can remove the additional checks from the conditions in shouldInstrument.js file.

This is work in progress, but I've wanted to hear your opinion regarding my modified version of testShouldInstrument function in the should_instrument.test.js file. Can you think of any cases that can break this? Is it better to just simply provide a normalized config as an argument every time?

I'm taking care of the failed unit tests right now, any clues how to approach this?

Test plan

Adjusted unit tests. If we are certain that shouldInstrument receives normalized config, I think it should be enough.

@lekterable lekterable force-pushed the lekterable:typing-and-normalized-config branch from caeb2ae to 3b48d87 Feb 4, 2019
@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Feb 5, 2019

Can you think of any cases that can break this?

Removing the property checks should be fine, ProjectConfig doesn't allow forceCoverageMatch, testPathIgnorePatterns etc to be undefined, looks like the only reason why it worked without them in the test was because it was missing @flow so everything was any

@lekterable lekterable force-pushed the lekterable:typing-and-normalized-config branch 3 times, most recently from e6565ec to 21f8476 Feb 5, 2019
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 7, 2019

@lekterable CI is unhappy. Don't hesitate to ask if you're stuck :)

@lekterable

This comment has been minimized.

Copy link
Contributor Author

lekterable commented Feb 9, 2019

@SimenB yeah I feel a little bit stuck because I can't figure out to which path argument the error is referring to, any clues?

@jeysal

This comment has been minimized.

Copy link
Collaborator

jeysal commented Feb 9, 2019

Yeah, that's a pretty obscure error. Looks like the added jest-config import causes problems because of the fs mock in the script_transformer.test. Looking at getCacheDirectory, it imports realpath-native. Adding realpath: jest.requireActual('fs').realpath and the same for realpathSync to the fs mock (like there already is for ReadStream and WriteStream) might work.
@SimenB better ideas? The script_transformer.test setup is not exactly getting prettier 😄

@lekterable lekterable force-pushed the lekterable:typing-and-normalized-config branch from 21f8476 to acee5a1 Feb 17, 2019
@lekterable lekterable changed the title [WIP]: Typing and normalized config [WIP]: Normalize config and remove unecessary checks Feb 18, 2019
@lekterable

This comment has been minimized.

Copy link
Contributor Author

lekterable commented Feb 18, 2019

Hey guys, I've started over again with this branch and I think I've found a bug in TestUtils.js could you take a look @SimenB and @jeysal? btw after using this makeProjectConfig instead of normalize there is no ugly error in the script_transformer.test anymore, just some snapshots to update which seem to make sense so thats pretty cool, the only thing that bothers me is weird error i get while running tests which is

Test suite failed to run

Cannot find module '@jest/transform' from 'generateEmptyCoverage.test.js'
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 18, 2019

Awesome! That error is since a new package we've added hasn't been symlinked yet. Just run yarn in the root of the repo and the error should go away 🙂

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 18, 2019

@lekterable if you could convert TestUtils to TypeScript at the same time, that'd be awesome!

Here's a diff you can apply:

diff --git i/TestUtils.ts w/TestUtils.ts
index 1c588f058..ba80887bd 100644
--- i/TestUtils.ts
+++ w/TestUtils.ts
@@ -3,15 +3,12 @@
  *
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
- *
- * @flow
  */
 
-'use strict';
-
-import type {GlobalConfig, ProjectConfig} from 'types/Config';
+// eslint-disable-next-line import/no-extraneous-dependencies
+import {Config} from '@jest/types';
 
-const DEFAULT_GLOBAL_CONFIG: GlobalConfig = {
+const DEFAULT_GLOBAL_CONFIG: Config.GlobalConfig = {
   bail: 0,
   changedFilesWithAncestor: false,
   changedSince: '',
@@ -67,7 +64,7 @@ const DEFAULT_GLOBAL_CONFIG: GlobalConfig = {
   watchman: false,
 };
 
-const DEFAULT_PROJECT_CONFIG: ProjectConfig = {
+const DEFAULT_PROJECT_CONFIG: Config.ProjectConfig = {
   automock: false,
   browser: false,
   cache: false,
@@ -124,7 +121,9 @@ const DEFAULT_PROJECT_CONFIG: ProjectConfig = {
   watchPathIgnorePatterns: [],
 };
 
-export const makeGlobalConfig = (overrides: Object = {}): GlobalConfig => {
+export const makeGlobalConfig = (
+  overrides: Partial<Config.GlobalConfig> = {},
+): Config.GlobalConfig => {
   const overridesKeys = new Set(Object.keys(overrides));
   Object.keys(DEFAULT_GLOBAL_CONFIG).forEach(key => overridesKeys.delete(key));
 
@@ -138,7 +137,9 @@ export const makeGlobalConfig = (overrides: Object = {}): GlobalConfig => {
   return {...DEFAULT_GLOBAL_CONFIG, ...overrides};
 };
 
-export const makeProjectConfig = (overrides: Object = {}): ProjectConfig => {
+export const makeProjectConfig = (
+  overrides: Partial<Config.ProjectConfig> = {},
+): Config.ProjectConfig => {
   const overridesKeys = new Set(Object.keys(overrides));
   Object.keys(DEFAULT_PROJECT_CONFIG).forEach(key => overridesKeys.delete(key));
 
@@ -149,5 +150,5 @@ export const makeProjectConfig = (overrides: Object = {}): ProjectConfig => {
     `);
   }
 
-  return {...DEFAULT_GLOBAL_CONFIG, ...overrides};
+  return {...DEFAULT_PROJECT_CONFIG, ...overrides};
 };
@lekterable

This comment has been minimized.

Copy link
Contributor Author

lekterable commented Feb 18, 2019

done @SimenB , let me know what you think 😄

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 18, 2019

You can fix flow by adding 2 $FlowFixMe: Converted to TS comments above the violations 🙂

@SimenB
SimenB approved these changes Feb 18, 2019
Copy link
Collaborator

SimenB left a comment

This is great, thanks!

Will merge when CI is happy 🙂

@lekterable lekterable changed the title [WIP]: Normalize config and remove unecessary checks Normalize config and remove unecessary checks Feb 18, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #7801 into master will decrease coverage by 0.05%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7801      +/-   ##
==========================================
- Coverage   65.42%   65.37%   -0.06%     
==========================================
  Files         255      255              
  Lines        9949     9940       -9     
  Branches     1039     1029      -10     
==========================================
- Hits         6509     6498      -11     
- Misses       3193     3194       +1     
- Partials      247      248       +1
Impacted Files Coverage Δ
packages/jest-transform/src/shouldInstrument.ts 88.23% <75%> (-7.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94576a5...371733b. Read the comment docs.

@SimenB SimenB merged commit bc51873 into facebook:master Feb 18, 2019
11 of 12 checks passed
11 of 12 checks passed
facebook.jest in progress
Details
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@lekterable lekterable deleted the lekterable:typing-and-normalized-config branch Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.