Refactor configs & transform #3376
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3376 +/- ##
=========================================
- Coverage 64.2% 64.2% -0.01%
=========================================
Files 176 176
Lines 6546 6532 -14
Branches 4 4
=========================================
- Hits 4203 4194 -9
+ Misses 2342 2337 -5
Partials 1 1
Continue to review full report at Codecov.
|
cpojer
force-pushed
the
refactor-configs
branch
from
e665d2d
to
9d0ec43
Compare
cpojer
force-pushed
the
refactor-configs
branch
from
9d0ec43
to
7040d4e
Compare
| .update('\0', 'utf8') | ||
| // Don't use the in-memory cache in watch mode because the .babelrc | ||
| // file may be modified. | ||
| .update(getBabelRC(filename, {useCache: !watch})) |
There was a problem hiding this comment.
Did it even work earlier?
There was a problem hiding this comment.
Yap this worked earlier but the code was terrible!
There was a problem hiding this comment.
Also, what's the idea behind moving getBabelRC into createTransformer? Just curious
There was a problem hiding this comment.
That's the reason I rewrote transform into ScriptTransformer. What we do now when we get a transformer, we check if createTransformer exists. If it does, we call that. We do this now once per Runtime instance (once per Test). This means every test will have its own babelrc file cache instead of having a single cache for the entirety of Jest's lifetime. Does that make sense?
| } | ||
|
|
||
| if (globalConfig.updateSnapshot === true) { | ||
| setConfig(contexts, {updateSnapshot: true}); |
There was a problem hiding this comment.
👍 the less config mutations the better
cpojer
force-pushed
the
refactor-configs
branch
from
7040d4e
to
bea6547
Compare
| let vm; | ||
|
|
||
| describe('transform', () => { | ||
| describe('ScriptTransformer', () => { |
There was a problem hiding this comment.
Let's also rename the test file (and corresponding snapshot) to ScriptTransformer-test.js for consistency.
| @@ -157,41 +158,39 @@ describe('transform', () => { | |||
| expect(fs.readFileSync).toBeCalledWith('/fruits/banana.js', 'utf8'); | |||
|
|
|||
| // in-memory cache | |||
| const response2 = transform('/fruits/banana.js', config).script; | |||
| const response2 = scriptTransformer.transform('/fruits/banana.js').script; | |||
| expect(response2).toBe(response); | |||
|
|
|||
There was a problem hiding this comment.
Shouldn't we make sure that the fs is not called again?
// in-memory cache
fs.readFileSync.mockClear();
const response2 = scriptTransformer.transform('/fruits/banana.js').script;
expect(response2).toBe(response);
expect(fs.readFileSync).not.toHaveBeenCalled();There was a problem hiding this comment.
We can assume if we have referential equality, that we didn't go to the filesystem and create another vm.Script instance.
cpojer
force-pushed
the
refactor-configs
branch
from
bea6547
to
ee6d34f
Compare
|
I renamed the transform-test.js file. Thanks for that recommendation and the thorough review! |
* Remove “updateSnapshot” from `ProjectConfig`, fix watch mode behavior. * Remove `verbose`, `watch`, `watchman`, `testNamePattern` from ProjectConfig. * Refactor transform.js into a ScriptTransformer
|
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. |
Summary
This diff starts to actually separate configs into two individual ones with distinct keys rather than overlaps. It requires a bunch of rewrites because some config fields were passed into the wrong places in the first place. For example, I made it so we passed "watch" to babel-jest, which should not know anything about how it is being used. I had to rewrite the entire transform pipeline (
transform.jstoScriptTransformer.js) to cache transform specific data (like the location of .babelrc) per test rather than globally. However, I think it was worth it. The new ScriptTransformer makes much more sense than the spaghetti file we had there before.There'll be more diffs on top of this, I recommend reviewing this in the order of the commits, with the last one being the biggest one. There'll be many more diffs to separate configs more.
Test plan
jest