Normalize config and remove unecessary checks

[lekterable]

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.

[jeysal]

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

[SimenB]

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

[lekterable]

@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]

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]

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]

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]

@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]

done @SimenB , let me know what you think 😄

[SimenB]

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

[SimenB]

This is great, thanks!

Will merge when CI is happy 🙂

[codecov-io]

Codecov Report

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

@@            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.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.