Allow setting a test environment per file #2859

SimenB · 2017-02-10T23:48:40Z

Summary

Fixes #2587. This is WIP, opening a PR now to get feedback on direction.

Test plan

Source:

export default (num) => num * 2;

Test:

/**
 * @testEnvironment jsdom
 */

import add from '../index.js';

test('add should work', () => {
  expect(add(2)).toEqual(4);
  document.createElement('a');
});

Command run without these changes:

$ ../jest/packages/jest/bin/jest.js --env node
 FAIL  __tests__/index.js
  ● add should work

    ReferenceError: document is not defined

      at Object.<anonymous> (__tests__/index.js:9:3)

  ✕ add should work (4ms)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        0.569s, estimated 3s
Ran all test suites.

Command run with these changes:

$ ../jest/packages/jest/bin/jest.js --env node
 PASS  __tests__/index.js
  ✓ add should work (4ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        1.032s
Ran all test suites.

SimenB · 2017-02-10T23:50:42Z

packages/jest-cli/src/runTest.js

-        }
+    }
+
+    /* $FlowFixMe */


https://github.com/facebook/jest/pull/2859/files?w=1 is recommended

SimenB · 2017-02-10T23:50:56Z

packages/jest-config/src/index.js

@@ -48,6 +49,7 @@ const readRawConfig = (argv, root) => {
 };

 module.exports = {
+  getTestEnvironment,


Is this OK?

codecov-io · 2017-02-11T00:26:27Z

Codecov Report

Merging #2859 into master will decrease coverage by -0.06%.
The diff coverage is 10.52%.

@@            Coverage Diff             @@
##           master    #2859      +/-   ##
==========================================
- Coverage   67.12%   67.07%   -0.06%     
==========================================
  Files         142      142              
  Lines        5126     5136      +10     
==========================================
+ Hits         3441     3445       +4     
- Misses       1685     1691       +6

Impacted Files	Coverage Δ
packages/jest-config/src/index.js	`0% <ø> (ø)`	✅
packages/jest-cli/src/runTest.js	`19.51% <10.81%> (+7.01%)`	✅

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 8c17c0f...e33ef93. Read the comment docs.

SimenB · 2017-02-22T18:46:27Z

@cpojer Added a test to this, should be ready for review

cpojer · 2017-03-02T14:06:01Z

docs/Configuration.md

+ * @testEnvironment jsdom
+ */
+
+test('use document in a mostly node environment', () => {


Should be "use jsdom in this test file"

cpojer · 2017-03-02T14:09:10Z

docs/Configuration.md

+ */
+
+test('use document in a mostly node environment', () => {
+  const ele = document.createElement('div');


cpojer · 2017-03-02T14:09:45Z

integration_tests/__tests__/test-environment-test.js

+const testFixturePackage = require('../test-environment/package.json');
+
+it('respects testEnvironment pragma', () => {
+  expect(testFixturePackage.jest.testEnvironment).toEqual('node');


I don't think you really need to check this. Can you remove this line?

It's there to prevent a false positive if the package.json is changed in the future. You don't think that's necessary?

Yeah, good point actually. Let's keep it then!

cpojer · 2017-03-02T14:09:55Z

integration_tests/test-environment/__tests__/env-test.js

+ */
+
+test('stub', () => {
+  const ele = document.createElement('div');


cpojer · 2017-03-02T14:10:44Z

integration_tests/test-environment/__tests__/env-test.js

+
+/**
+ * @testEnvironment jsdom
+ */


The file should look like this:

/** * Copyright (c) 2014-present, Facebook, Inc. All rights reserved. * * This source code is licensed under the BSD-style license found in the * LICENSE file in the root directory of this source tree. An additional grant * of patent rights can be found in the PATENTS file in the same directory. * * @testEnvironment jsdom */ /* eslint-env browser*/

cpojer · 2017-03-02T14:11:11Z

packages/jest-cli/src/runTest.js

 const {
  Console,
  NullConsole,
  setGlobal,
 } = require('jest-util');
 const getConsoleOutput = require('./reporters/getConsoleOutput');

+const testEnvironmentPragmaRegex = /@testEnvironment\s+(.*)/;


There is a docblock parser in jest-haste-map. Please let's use that instead. Feel free to extract it into a separate package "jest-docblock".

Also, I was thinking we could call this @jest-environment instead. That way we can add more of these in the future with the same prefix :)

Ah, nice! Will do

Didn't see the comment about @jest-environment (no idea how I missed it), will push a change to it

cpojer · 2017-03-02T14:14:44Z

packages/jest-cli/src/runTest.js

 const BufferedConsole = require('./lib/BufferedConsole');
+const promisify = require('./lib/promisify');
 const {
  Console,
  NullConsole,
  setGlobal,
 } = require('jest-util');
 const getConsoleOutput = require('./reporters/getConsoleOutput');


Please keep the requires consistent. Here is how it should work with Jest:

const Capitalized = require('MyClass'); const {getTestEnvironment} = require('jest-config'); const lowercase = require('myfunction');

We have this weird spacing rule at FB, let's keep that for now :)

I think it's correct now...

cpojer · 2017-03-02T14:20:40Z

packages/jest-cli/src/runTest.js

-  const TestRunner = require(config.testRunner);
-  /* $FlowFixMe */
-  const ModuleLoader = require(config.moduleLoader || 'jest-runtime');
+  return promisify(fs.readFile)(path, 'utf8').then(data => {


Can you switch this to fs.readFileSync?

I'm a bit worried about the fact that this will end up reading every test file twice and we could potentially change things around by passing a fake FS into ModuleLoader that has a cache, like this:

new ModuleLoader({
…,
cacheFS: {
[path]: source,
},
});

and then later pass that source to transform inside of jest-runtime so that we don't read the same file twice. What do you think?

Why sync? The function already returns a promise.

But yes, I agree the file should only have to be read once, I'll see what I can do 😄

Because there is no other work that can be done in Jest at the same time, so there is no benefit in using async code for reading the file :)

cpojer

This looks pretty good! I left a few comments inline – would you mind making another iteration?

SimenB · 2017-03-02T16:09:48Z

@cpojer Pushed with the changes requested except for the reading of the source file twice. From what I can see in the code, Runtime::requireModule just does require, which is not the same as reading the file itself. How would I populate the cache?

I can require(path) and do toString() on it, but that will make node choke if it's not been transpiled yet.

cpojer · 2017-03-03T15:11:11Z

packages/jest-docblock/package.json

@@ -0,0 +1,10 @@
+{
+  "name": "jest-docblock",
+  "version": "19.0.0",


Can you set the version here (and where it is used) to 19.0.2? :)

cpojer · 2017-03-03T15:11:24Z

packages/jest-cli/src/runTest.js

+    data = fs.readFileSync(path, 'utf8');
+  } catch (e) {
+    return Promise.reject(e);
+  }


cpojer · 2017-03-03T15:11:53Z

packages/jest-cli/src/runTest.js

+    return Promise.reject(e);
+  }
+  let testEnvironment = config.testEnvironment;
+


code style: can you remove this newline and move the const above the let? :)

cpojer · 2017-03-03T15:13:31Z

packages/jest-cli/src/runTest.js

+
+const fs = require('fs');
+const docblock = require('jest-docblock');
+const {getTestEnvironment} = require('jest-config');


I think this one goes on top of all the requires (because of the {)

You should add eslint-plugin-imports on this stuff 😄

Would love to use a lint plugin to enforce the current rules we have. Mind sending a separate PR for that? :)

I can give it a try, at least. No promises

cpojer · 2017-03-03T15:14:23Z

packages/jest-cli/src/runTest.js

+  }
+  let testEnvironment = config.testEnvironment;
+
+  const testEnvironmentDocblock = docblock.parse(docblock.extract(data));


could we just do:

const customEnvironment = docblock.parse(docblock.extract(data))['jest-environment']

and then re-use that? We only need that one prop :)

I had it that way, but your way is more than 80 chars wide...

const parsedDocblock = docblock.parse(docblock.extract(data)); const customJestEnvironment = parsedDocblock['jest-environment'];

?

Or add a method to docblock, extractAndParse? I'm guessing they're separate for testing purposes, you wouldn't use one without the other in "real" code, would you?

I like the code example up there, maybe just call it customEnvironment instead? :)

So

const parsedDocblock = docblock.parse(docblock.extract(testSource)); const customEnvironment = parsedDocblock['jest-environment'];

?

Yeah that seems reasonable! :)

cpojer · 2017-03-03T15:16:24Z

Nice! I guess we could make the change to jest-rutnime in a follow-up. The idea was to change jest-runtime to accept a cacheFS property in the constructor which is just a map from path -> source. Then inside of _execModule we could pass on the source that we already read to the transform function somehow. Basically we could do transform(…, {source: this._cacheFS[pathToFile] || null}) – does that make sense?

SimenB · 2017-03-03T15:25:24Z

Runtime changes in a separate commit makes sense.
And yes, what you say makes sense, I just don't know the code enough to be able to thread it through properly. Want me to add a TODO comment?

cpojer · 2017-03-03T15:44:52Z

Not a todo, but maybe an issue here that links to this PR :)

Fixes jestjs#2587

SimenB · 2017-03-03T15:55:25Z

@cpojer OK, everything addressed except for reading the file twice (I can open up an issue for it)

cpojer · 2017-03-03T16:13:45Z

Really solid work @SimenB. Thank you!

danieldiekmeier · 2017-03-03T16:27:10Z

@SimenB Thanks for doing the work! I'm excited to try it out soon. :)

SimenB · 2017-03-03T16:28:39Z

Opened up #3060 for the fs cache we discussed here

@SimenB Thanks for doing the work! I'm excited to try it out soon. :)

We "need" this at work, so just happy to see it done 😄

* Extract docblock into its own module * Allow setting a test environment per file Fixes jestjs#2587

jcksncllwy · 2018-08-07T23:08:43Z

I have a component that does some fancy work with image processing and canvases that requires images to load in the test. For this, I'm setting {resources: "usable"} in the testEnvironmentOptions and it works great. However, all other tests really don't need to load resources.

Would it be possible to somehow use this per-file test environment specification for testEnvironmentOptions as well?

thymikee · 2018-08-08T06:29:17Z

@jcksncllwy you can create another project in Jest config that will only match certain tests and load them with your testEnvironmentOptions. I see how this would be nice to have in docblock, but that's yet another option to maintain.

github-actions · 2021-05-12T11:06:42Z

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.

facebook-github-bot added the CLA Signed ✔️ label Feb 10, 2017

SimenB mentioned this pull request Feb 10, 2017

Change testEnvironment on a per test basis #2587

Closed

SimenB commented Feb 10, 2017

View reviewed changes

SimenB force-pushed the env-per-test branch from a43e1bb to e33ef93 Compare February 13, 2017 06:55

SimenB force-pushed the env-per-test branch from e33ef93 to 84e6c9d Compare February 22, 2017 18:45

SimenB changed the title ~~[WIP] Allow setting a test environment per file~~ Allow setting a test environment per file Feb 22, 2017

SimenB force-pushed the env-per-test branch from 84e6c9d to 5c2ebce Compare February 23, 2017 15:22

cpojer reviewed Mar 2, 2017

View reviewed changes

cpojer requested changes Mar 2, 2017

View reviewed changes

SimenB force-pushed the env-per-test branch from 5c2ebce to caa8ff0 Compare March 2, 2017 16:00

SimenB force-pushed the env-per-test branch 4 times, most recently from f4b2b7b to a19e8a6 Compare March 2, 2017 17:13

cpojer reviewed Mar 3, 2017

View reviewed changes

packages/jest-cli/src/runTest.js Outdated

data = fs.readFileSync(path, 'utf8');

} catch (e) {

return Promise.reject(e);

}

Copy link

Member

cpojer Mar 3, 2017

Choose a reason for hiding this comment

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

nice!

cpojer reviewed Mar 3, 2017

View reviewed changes

Extract docblock into its own module

5b48e26

SimenB force-pushed the env-per-test branch from a19e8a6 to e76152a Compare March 3, 2017 15:29

Allow setting a test environment per file

aebc4bf

Fixes jestjs#2587

SimenB force-pushed the env-per-test branch from e76152a to aebc4bf Compare March 3, 2017 15:54

cpojer merged commit 62c4e9a into jestjs:master Mar 3, 2017

SimenB deleted the env-per-test branch March 3, 2017 16:14

SimenB mentioned this pull request Mar 3, 2017

Cache the source of a test file to avoid reading it twice #3060

Closed

thymikee mentioned this pull request Mar 6, 2017

Jest and Mongoose not working well together #3045

Closed

skovhus pushed a commit to skovhus/jest that referenced this pull request Apr 29, 2017

Allow setting a test environment per file (jestjs#2859)

6571750

* Extract docblock into its own module * Allow setting a test environment per file Fixes jestjs#2587

SimenB mentioned this pull request Apr 30, 2017

Use eslint-plugin-import #3423

Merged

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

Allow setting a test environment per file (jestjs#2859)

0a77b0d

* Extract docblock into its own module * Allow setting a test environment per file Fixes jestjs#2587

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Allow setting a test environment per file #2859

Allow setting a test environment per file #2859

Conversation

SimenB commented Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 11, 2017 • edited Loading

Codecov Report

SimenB commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

SimenB commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Mar 3, 2017

SimenB commented Mar 3, 2017

cpojer commented Mar 3, 2017

SimenB commented Mar 3, 2017

cpojer commented Mar 3, 2017

danieldiekmeier commented Mar 3, 2017

SimenB commented Mar 3, 2017

jcksncllwy commented Aug 7, 2018 • edited Loading

thymikee commented Aug 8, 2018

github-actions bot commented May 12, 2021

SimenB commented Feb 10, 2017 •

edited

Loading

codecov-io commented Feb 11, 2017 •

edited

Loading

SimenB Mar 3, 2017 •

edited

Loading

jcksncllwy commented Aug 7, 2018 •

edited

Loading