Feature: Enable defining tests/suites asynchronously (inside of tests or lifecycle hooks)

[niieani]

Context and motivation

It's currently not possible to add tests to a suite once the suite has been started.

While migrating Webpack's test suite from Mocha to Jest, me and @ooflorent have encountered this issue, because of the pattern Webpack uses to test its builds:

// pseudo-code to illustrate:
test('compile', (done) => {
  webpack.compile((err, compiledFileContent) => {
    if (err) done(err);

    const fn = vm.runInThisContext(
      `(function(require, module, exports, __dirname, __filename, it, expect) {
        // the compiled file contains test definitions,
        // which we want to add to the suite:
        ${compiledFileContent}
      })`
	);

    fn();
  })
});

The above call to webpack compiles a file containing a set of test definitions, then runs that file in the context of the suite. This pattern does work in Mocha (albeit by manually executing the suite), and I believe it's a very good solution to the problem at hand (testing compiled code).

I couldn't find anything in the jest ecosystem that would work just as well, without resorting to hacks, or having two-step testing, with separate configurations, test file generated from within tests, etc., which would add unnecessary complexity to the system.

Currently in jest, calling it or test within another it throws an error:

https://github.com/facebook/jest/blob/f6f0c9c8b8c418c98b49ca3bf90e248756072e74/packages/jest-jasmine2/src/jasmine/Env.js#L452-L462

Interestingly, the same check isn't applied to fit (forced test), but I get that it's probably because they're rarely used and not well documented. Just as the warning in the it definition states, these tests would not run. Similarly, it was possible to create suites with describe inside of a test, but it wouldn't run).

As a side note, I did find a hacky way to force such a test to run, but it runs the test without the blessing of the jests runner:

it('parent', (done) => {
  // "fit" because "it" throws:
  const spec = fit('child', () => {})
  spec.execute(done);
})

This was really hacky so I did a little digging into jest's internals.

It turned out there was a pretty simple way to enable:

• defining tests/suites inside of tests
• defining tests/suites in lifecycle hooks
• defining tests asynchronously

It all boils down to adding an extra pass to the tree processor, which executes children added during the initial run.

In this PR, I've also unified/cleaned-up the way the setup and teardown is done for the top suite - it's now done by the tree processor, just as for other suites, instead of having the code manually duplicated, as it was before.

All the changes in the PR are contained in the jest-jasmine2 package.

Finally, note that nesting tests doesn't add children to the currently running test (since tests don't have children), but simply schedules that test to run within the context of the current suite. Similarly, defining a suite inside of a test would make it run just before the currently scheduled suite's afterAll hook (if one exists).

Test plan

I've added a comprehensive test for all the newly supported scenarios:

• nested async test definition
• nested sync test definition
• doubly nested (both async and sync scenarios)
• adding tests to suite within beforeAll

[codecov-io]

Codecov Report

Merging #5673 into master will decrease coverage by 0.1%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #5673      +/-   ##
==========================================
- Coverage   63.16%   63.05%   -0.11%     
==========================================
  Files         216      216              
  Lines        7913     7926      +13     
  Branches        3        3              
==========================================
  Hits         4998     4998              
- Misses       2914     2927      +13     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-jasmine2/src/tree_processor.js	0% <0%> (ø)	⬆️
packages/jest-jasmine2/src/jasmine/Env.js	0% <0%> (ø)	⬆️

---

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 f6f0c9c...44d71d1. Read the comment docs.

[dyst5422]

Would this cover the case of tests asyncronously being defined inside of a describe?

The case I'm thinking of is

describe(() => {
  it.skipIf(() => Promise<boolean>, 'testname', () => {
    // Test assertions here
  })
})

Where the simpleton's version of skipIf looks like

async function skipIf(condition, name, test) {
  if (await condition) {
    it.skip(name, test);
  } else {
    it(name, test);
  }
}

[niieani]

@dyst5422 yes, although the skipIf would have to be defined in a beforeAll or inside of another test, since the execution of the describe callback is synchronous.

[niieani]

@cpojer @SimenB any rough ETA on when this could be reviewed/merged?
It's currently blocking Webpack from getting migrated from mocha to jest (webpack/webpack#6565).

Huge thanks! 🙇

[TheLarkInn]

@cpojer @SimenB is there any way we can assist in the progress of this? We would love to be able to land Jest into webpack for our tests. Happy to help in anyways possible!

[niieani]

@TheLarkInn I have a plan. 👍 Spoke with @ooflorent about the details in the last two days. With a few small changes to webpack's jest-migrated branch and the release of jest v23.0, we'll be able to make the switch.

As for this PR, I'll rebase on top of current master so it's clear what this PR really does (the refactor from #5885 was coupled with enabling async testing), and we can discuss the best way to go forward with async tests with the jest team.

[SimenB]

A quick rebase would be nice to clean up the diff.

Our main concern is that this will mess with static analysis, making other features such as #5705, linting or other future improvements less useful.

It was pointed out however that generating tests is already possible, and the question is whether being able to do it async worsens the situation or keeps the status quo.
Defining a test within another test seems yucky though.

It'll be interesting to see the alternative approach talked about above :)

[SimenB]

Another issue might be how it affects things like https://facebook.github.io/jest/docs/en/cli.html#testnamepattern-regex

[rickhanlonii]

Curious how this would interact with coverage as well

@niieani does jest@beta have everything you need to give your new strategy a go?

[niieani]

@rickhanlonii I see the last released alpha is from the 26th March. I need something after my refactor (815c8bd) since that fixes the way suites are set-up/teared-down in Env.execute (previously the top suite was being setup separately).

My strategy still feels a bit suboptimal, but does work well and is simple enough to understand. In fact, it's very similar to how this used to work in mocha.

it('compiles', async () => {
    const exportedTests = await compileAndGetExportedTests();
	const asyncSuite = describe("exported tests", () => {
		exportedTests.forEach(
			({ title, fn, timeout }) =>
				fn
					? fit(title, fn, timeout)
					: fit(title, () => {}).pend("Skipped")
					// ^ forced test, as that one doesn't throw when defined asynchronously
		);
	});

	// this is where the magic happens:
	await jasmine
		.getEnv()
		.execute([asyncSuite.id], asyncSuite);
		// ^ executes only the tree of asyncSuite
})

I have the webpack's codebase open with the jest from master built and yarn linked, and have applied the above change to the test files that needed it.

The tests run, but the big difference is, for jest it only takes 2 minutes 35 seconds to run all the 10713 tests, while mocha previously took 13 minutes (running on the same machine). :)

[rickhanlonii]

Discussed with @niieani - closing for now, we added what webpack needed in a different PR

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