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

Add Async Test Environment APIs #4506

Merged
merged 1 commit into from
Oct 13, 2017

Conversation

xfumihiro
Copy link
Contributor

@xfumihiro xfumihiro commented Sep 19, 2017

Summary

Adding Async Test Environment APIs based on @cpojer's comments on #3420 & #3832
Fixes #3420

Test plan

Example Puppeteer Environment

class PuppeteerEnvironment extends NodeEnvironment {
  constructor(config) {
    super(config)
  }

  async setup() {
    await super.setup()
    this.global.__BROWSER__ = await puppeteer.connect({
      browserWSEndpoint: this.global.browserWSEndpoint
    })
  }

  async teardown() {
    await this.global.__BROWSER__.close()
    await super.teardown()
  }

  runScript(script) {
    return super.runScript(script)
  }
}

@@ -100,7 +100,11 @@ export default function runTest(
mapCoverage: globalConfig.mapCoverage,
});
const start = Date.now();
return testFramework(globalConfig, config, environment, runtime, path)
return (environment.setup ? environment.setup() : (async () => ({}))())
Copy link
Member

@SimenB SimenB Sep 19, 2017

Choose a reason for hiding this comment

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

return (environment.setup ? environment.setup() : Promise.resolve({}))

can be used 2 other places as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! TIL :)

if (globalConfig.logHeapUsage) {
if (global.gc) {
global.gc();
Promise.resolve()
Copy link
Member

Choose a reason for hiding this comment

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

Could we instead use the async modifier on the promise callback and use await? I think that would be tons clearer for this code!

@xfumihiro xfumihiro force-pushed the async_test_environment branch 4 times, most recently from c9989c9 to f7e61f9 Compare September 20, 2017 11:50
@codecov-io
Copy link

codecov-io commented Sep 20, 2017

Codecov Report

Merging #4506 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4506      +/-   ##
==========================================
- Coverage   57.13%   57.11%   -0.03%     
==========================================
  Files         195      194       -1     
  Lines        6569     6568       -1     
  Branches        3        3              
==========================================
- Hits         3753     3751       -2     
- Misses       2816     2817       +1
Impacted Files Coverage Δ
packages/jest-editor-support/src/Snapshot.js 98.07% <ø> (ø) ⬆️
packages/jest-runner/src/run_test.js 2.27% <0%> (+0.05%) ⬆️
packages/jest-environment-node/src/index.js 57.69% <0%> (-4.81%) ⬇️
packages/jest-environment-jsdom/src/index.js 41.37% <0%> (-3.07%) ⬇️
packages/jest-snapshot/src/plugins.js 100% <0%> (ø) ⬆️
packages/jest-snapshot/src/mock_serializer.js

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 8a427eb...a259379. Read the comment docs.

@xfumihiro
Copy link
Contributor Author

@cpojer any thing to improve?

.then((result: TestResult) => {
return (environment.setup
? environment.setup()
: Promise.resolve({})).then(async testSetup => {
Copy link
Member

Choose a reason for hiding this comment

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

This is really hard to read. Can we not use a ternary here? Thanks!

throw err;
}),
);
if (environment.teardown) await environment.teardown();
Copy link
Member

Choose a reason for hiding this comment

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

this needs curly braces, wondering why CI didn't catch that :O

Copy link
Member

Choose a reason for hiding this comment

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

// Delay the resolution to allow log messages to be output.
return new Promise(resolve => setImmediate(() => resolve(result)));
} catch (err) {
if (environment.teardown) await environment.teardown();
Copy link
Member

Choose a reason for hiding this comment

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

curly braces as well pls

@xfumihiro
Copy link
Contributor Author

@cpojer any thing else?

return testFramework(globalConfig, config, environment, runtime, path)
.then((result: TestResult) => {
return environment.setup().then(async testSetup => {
setGlobal(environment.global, 'setup', testSetup);
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be here, we don't want to expose this function on the global object.

Copy link
Contributor Author

@xfumihiro xfumihiro Sep 30, 2017

Choose a reason for hiding this comment

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

For some use case(i.e, the Test Plan of this PR) it's necessary to set global variables in the setup function for later usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe

return environment.setup().then(async setupGlobal => {
    setGlobal(environment.global, 'setup', setupGlobal);

would be better

Copy link
Member

Choose a reason for hiding this comment

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

I prefer not to do this. The change in this PR should not automatically add things to the global scope. The setup function in the environment has access to the global scope and we can leave it to the responsibility of the user.

@@ -31,6 +31,8 @@ declare class $JestEnvironment {
},
testFilePath: string,
moduleMocker: ModuleMocker,
setup(): Promise<Global>,
Copy link
Member

Choose a reason for hiding this comment

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

Can you change the signature to Promise? I don't think we need to return the global here.

cpojer
cpojer previously requested changes Sep 30, 2017
Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Two more changes to clean up this feature please. It would also be great if you could add an integration test for this, with a custom test framework.

@xfumihiro xfumihiro force-pushed the async_test_environment branch 2 times, most recently from 5fb8947 to 8a50998 Compare October 3, 2017 08:50
@xfumihiro
Copy link
Contributor Author

@cpojer can you rebulid the failing ci-tests? It should be working.

@SimenB
Copy link
Member

SimenB commented Oct 4, 2017

@xfumihiro rebuild didn't help. Have you tested on node 4 and/or 6? Node 8 passes CI, so if you've just tested on latest node locally that might explain it.

@devsnek
Copy link

devsnek commented Oct 4, 2017

wouldn't you want runScript to be async as well? for the puppeteer example you would want scripts to run in the browser (page.evaluate) which is async.

@@ -14,10 +14,9 @@ const runJest = require('../runJest');
const testFixturePackage = require('../test-environment/package.json');

it('respects testEnvironment docblock', () => {
expect(testFixturePackage.jest.testEnvironment).toEqual('node');
Copy link
Member

Choose a reason for hiding this comment

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

can you create a new test instead of modifying an existing one?

@xfumihiro
Copy link
Contributor Author

@devsnek not sure if this should be an issue as I've ran puppeteer tests in my own app successfully.
Once having the browser instance from the setup hook, I thought it would be working just fine in Jest async test case.🤔

@xfumihiro xfumihiro force-pushed the async_test_environment branch 2 times, most recently from 49bee97 to 71fda69 Compare October 5, 2017 03:05
@devsnek
Copy link

devsnek commented Oct 5, 2017

@xfumihiro right but it would be cooler to just do that at the environment level, allowing a better test flow for future environments whatever they might be. you could also make it so runScript takes a string intead of vm.Script, for even more flexibility, although that might be out of the scope of this pr.

result.memoryUsage = process.memoryUsage().heapUsed;
}
// Delay the resolution to allow log messages to be output.
return new Promise(resolve => setImmediate(() => resolve(result)));
Copy link
Member

Choose a reason for hiding this comment

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

this can probably be

await new Promise(resolve => setImmediate(resolve));
return result;

No idea if it affects the stacktraces of matchers, though.

Feel free to ignore!

@cpojer cpojer merged commit d1cb395 into jestjs:master Oct 13, 2017
@cpojer
Copy link
Member

cpojer commented Oct 13, 2017

Thank you.

@cpojer
Copy link
Member

cpojer commented Oct 13, 2017

Published 21.3.0-beta.2.

@rotemmiz
Copy link

This is the last piece of the puzzle , we can now integrate Jest support in Detox!
Thank you guys!

@orta
Copy link
Member

orta commented Oct 13, 2017

Great PR 👍

@delgermurun
Copy link

It still hasn't released to stable version, then why it is documented?

http://facebook.github.io/jest/docs/en/configuration.html#testenvironment-string

class CustomEnvironment extends NodeEnvironment {
  constructor(config) {
    super(config);
  }

  async setup() {
    await super.setup();
    await someSetupTasks();
  }

  async teardown() {
    await someTeardownTasks();
    await super.teardown();
  }

  runScript(script) {
    return super.runScript(script);
  }
}

You guys should've merge document's change after releasing it.

@SimenB
Copy link
Member

SimenB commented Oct 26, 2017

It should include a note that's it's not available until 21.3.0. PR welcome!

@xfumihiro
Copy link
Contributor Author

Done #4765

@rdeltour
Copy link

I'm experimenting with an async environment (great PR! 👍), but apparently it doesn't run on Node v6:

   /home/travis/build/daisy/ace/tests/jest-env-puppeteer.js:10
      async setup() {
            ^^^^^   
    SyntaxError: Unexpected identifier    
      at node_modules/jest-runner/build/run_test.js:60:29

Will async environments be limited to versions of Node supporting async/await?

@SimenB
Copy link
Member

SimenB commented Oct 28, 2017

Just do return setup().then... instead of async/await

@rdeltour
Copy link

Just do return setup.then... instead of async/await

Doh. Of course, makes sense. Thanks!

@vajahath
Copy link

I'm facing some issues with this async environment. But can't determine if the hooks are properly working.
here is the abstract:

reset.js

require('ts-node/register'); // cz, the below required file is written in typescript
const resetDB = require('./reset-database');

module.exports = function() {
	return resetDB(); // returns Promise
};
/* I tried running the below code to verify this file is working. It is working */
// resetDB()
// 	.then(() => console.log('ok'))
// 	.catch(err => console.log(err));

jest-env.js

const NodeEnvironment = require('jest-environment-node');
const resetDatabase = require('./reset');

class CustomEnvironment extends NodeEnvironment {
	constructor(config) {
		super(config);
	}

	async setup() {
		await super.setup();
		console.log('setup running') // not prinitng
		await resetDatabase();
	}

	async teardown() {
		await resetDatabase();
		await super.teardown();
	}

	runScript(script) {
		return super.runScript(script);
	}
}

module.exports = CustomEnvironment;

these hooks are not 'properly' working for me. I can't see any consoles or database resets either. How do I debug this? Is it a bug?

package.json abstract

	"jest": {
		"globals": {
			"ts-jest": {
				"tsConfigFile": "tsconfig.json"
			}
		},
		"moduleFileExtensions": [
			"ts",
			"js"
		],
		"transform": {
			"^.+\\.(ts)$": "ts-jest"
		},
		"testMatch": [
			"**/tests/**/*.spec.ts"
		],
		"testEnvironment": "<path to jest-env.js>" // <--
	}

@SimenB
Copy link
Member

SimenB commented Nov 24, 2017

Are you using the beta of jest?

@imballinst
Copy link

imballinst commented Nov 30, 2017

Hello, first of all, thanks for the great PR! I want to ask a question, do constructor, setup, teardown and runScript run only once across all test files or once between test files? Because in my setup (I'm not sure though if I'm doing this right):

const NodeEnvironment = require('jest-environment-node');
const MongoDriver = require('../mongo/driver');

class CustomEnvironment extends NodeEnvironment {
  constructor(config) {
    super(config);

    console.log('halo');
  }

  setup() {
    return super.setup()
      .then(() => {
        // setup tasks
        console.log('halo1');
      });
  }

  teardown() {
    const teardownTask = new Promise((resolve, reject) => {
      console.log('halo2');
      resolve((MongoDriver.openDBConnection()));
    });

    return teardownTask.then(() => super.teardown());
  }

  runScript(script) {
    return super.runScript(script);
  }
}

module.exports = CustomEnvironment;

When the tests are running, these halos are always printed. I thought it should run only once for all tests. Am I missing something here? Using "jest": "^21.3.0-beta.2", in my package.json with Node v6.11.5. Thank you for explanation.

@xfumihiro
Copy link
Contributor Author

@imballinst Jest's test environments are "sandboxed" so this is normal.
I've submit a PR for global setup/teardown.

@imballinst
Copy link

@xfumihiro alright, thanks man!

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async Test Environment API