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

Allow for describe to return a promise #2235

Closed
ianstormtaylor opened this issue Dec 6, 2016 · 43 comments
Closed

Allow for describe to return a promise #2235

ianstormtaylor opened this issue Dec 6, 2016 · 43 comments

Comments

@ianstormtaylor
Copy link

Do you want to request a feature or report a bug?

Request a feature.

What is the current behavior?

Right now it seems like async functions (or promise-returning functions in general) can only be passed to it and the before*/after* functions for setting up tests.

What is the expected behavior?

It would be nice if describe could also allow for passing an async function for consistency.

In my case, I'm running through a whole bunch of fixtures in a directory and automatically generating a bunch of tests. And to do so, I want to use await fs.readdir(path) inside describe to get a list of the fixtures, instead of having to fallback to using fs.readdirSync(path).

@cpojer
Copy link
Member

cpojer commented Dec 6, 2016

I don't think that makes any sense. Describe is just used for grouping, you don't need to use it and most of the time it isn't useful. You can make everything else async which is what is recommended for tests.

@cpojer cpojer closed this as completed Dec 6, 2016
@cpojer
Copy link
Member

cpojer commented Dec 6, 2016

Happy to discuss this further, just closing to manage the queue but convince me/us and send a PR and we'll see what happens :)

@ianstormtaylor
Copy link
Author

I agree that it's for grouping, but I think as far as optimal developer experience goes it feels very intuitive to add "group-specific logic" inside of the describe function. And when that logic is async, it also feels intuitive to be able to use the same async-passing API as for all of the other Jest functions that are intermingled with describe.

The most obvious example to me is tests that are generated from the directory structure, usually because of the ease of creating fixtures. For example:

import fs from 'fs'

describe('transforms', () => {
  const dir = resolve(__dirname, 'fixtures')
  const transforms = fs.readdirSync(dir)

  beforeEach(() => {
    synchronousReset()
  })

  for (const transform of transforms) {
    it(transform, () => {
      const testDir = resolve(__dirname, 'fixtures', transform)
      const fn = require(testDir).default
      const input = fs.readFileSync(resolve(testDir, 'input.txt'), 'utf8')
      const output = fs.readFileSync(resolve(testDir, 'output.txt'), 'utf8')
      assert.equal(fn(input), output)
    })
  }
})

In the example, you have a series of fixture folders each with their own input.txt, output.txt and index.js transformer test function. This is a fairly common fixture-oriented testing practice.

Imagine now a developer was going to migrate that to async functions, since they heard that Jest had built-in support for promises so they can speed up their tests by not using the fs.*Sync methods...

It feels very intuitive to do this:

---import fs from 'fs'
+++import fs from 'fs-promise'

---describe('transforms', () => {
+++describe('transforms', async () => {
     const dir = resolve(__dirname, 'fixtures')
---  const transforms = fs.readdirSync(dir)
+++  const transforms = await fs.readdir(dir)

---  beforeEach(() => {
+++  beforeEach(async () => {
---    synchronousReset()
+++    await reset()
     })

     for (const transform of transforms) {
---    it(transform, () => {
+++    it(transform, async () => {
         const testDir = resolve(__dirname, 'fixtures', transform)
         const fn = require(testDir).default
---      const input = fs.readFileSync(resolve(testDir, 'input.txt'), 'utf8')
+++      const input = await fs.readFile(resolve(testDir, 'input.txt'), 'utf8')
---      const output = fs.readFileSync(resolve(testDir, 'output.txt'), 'utf8')
+++      const output = await fs.readFile(resolve(testDir, 'output.txt'), 'utf8')
         assert.equal(fn(input), output)
       })
     }
   })

Resulting in:

import fs from 'fs-promise'

describe('transforms', async () => {
  const dir = resolve(__dirname, 'fixtures')
  const transforms = await fs.readdir(dir)

  beforeEach(async () => {
    await reset()
  })

  for (const transform of transforms) {
    it(transform, async () => {
      const testDir = resolve(__dirname, 'fixtures', transform)
      const fn = require(testDir).default
      const input = await fs.readFile(resolve(testDir, 'input.txt'), 'utf8')
      const output = await fs.readFile(resolve(testDir, 'output.txt'), 'utf8')
      assert.equal(fn(input), output)
    })
  }
})

In that case, the user doesn't need to be aware that describe is "special" because it is run at the very beginning synchronously, and that even though it co-exists with all of the other Jest-related functions, it doesn't act in the same way.

That being said, I don't know the internals of Jest. To me it seems like the reason describe is currently synchronous is purely because "that's how it has always been", but there might be some valid architectural trade-off that makes it that way, which is fair if so. If there isn't anything specifically requiring it to be that way though, it seems like it would be more user-friendly not keep that limitation on it, so that it matches the other functions users are used to.

If you still disagree, I'd be curious how you'd solve my current use case. My case is slightly more complex that the example in that I have nested describe's for fixtures. I'm doing this for: https://github.com/ianstormtaylor/slate/tree/master/test/transforms

@ianstormtaylor
Copy link
Author

I guess put differently what I'm asking for architecturally is for the setup phase to be able to be asynchronous, which I can see might be a big ask. I just think it would be less limiting and end up with a more intuitive interface that way.

It would eliminate problems like #949 potentially too, which I assume was a similar use case, except in the external files setup realm.

@cpojer
Copy link
Member

cpojer commented Dec 7, 2016

You can always have side-effectful setup behavior in beforeEach, like:

let foo;
beforeEach(async () => foo = await bar());

We are hoping to one day rewrite Jasmine and remove it from Jest but that is still pretty far out. If you can submit a PR to add async support to "describe" and hack Jasmine to do that, I'd welcome it.

@faceyspacey
Copy link

faceyspacey commented Dec 30, 2016

+1

async consistency between describe and it is the best DX. using beforeEach or beforeAll to assign to a variable in the outer closure is sub-optimal.

Basically beforeAll becomes superfluous if describe supports async. More importantly the result is way cleaner code. For example, a common pattern for react + redux apps is to to setup the store ahead of time using beforeAll and a closure exactly like @cpojer mentioned, but ideally if describe supported async, it would be done like this:

describe('<MyComponent />', async () => {
  const store = configureStore()
  await store.dispatch(loadSomething()) //what most apps gotta do to setup the store

  it('does something', () => {
    //do something with <MyComponent /> and store
  })

  it('uses store again', () => {
    //do something different with <MyComponent /> and store
  })
})

Jest being the killer testing tool it is for React, tailoring for this pattern for Redux (which 90% of all serious react apps use) makes a lot of sense.

2 cents

@faceyspacey
Copy link

@cpojer can you provide me some quick direction (maybe just a list of relevant file names and functions) to look at to make describe async? Any other pitfalls you're aware of would be much appreciated.

@cpojer
Copy link
Member

cpojer commented Jan 3, 2017

This is inside of jest-jasmine2, inside of the vendor folder directly inside of Jasmine. As we are hoping to move away from Jasmine, feel free to try to rewrite this and send a PR :)

@mohsen1
Copy link

mohsen1 commented May 15, 2017

I think it does make sense to let describe callback be async. For example in my code there is no way to use beforeAll to achieve what's here:

for (const [testFolderName, getFactory] of transformToFolderMap) {
    describe(testFolderName, async () => {
        for (const folderName of await readDirectory(path.join(__dirname, testFolderName))) {
            const folder = path.join(path.join(__dirname, testFolderName, folderName));
            if (fs.statSync(folder).isDirectory()) {
                const inputPath = path.join(folder, 'input.tsx');
                const output = await readFile(path.join(folder, 'output.tsx'));
                const result = compile(inputPath, getFactory);
                it(folderName, () => {
                    expect(stripEmptyLines(result)).toEqual(stripEmptyLines(output));
                });
            }
        }
    });
}

@flushentitypacket
Copy link

Why was this closed?

@ianstormtaylor
Copy link
Author

ianstormtaylor commented Sep 11, 2017

@flushentitypacket it was mentioned by @cpojer upstream, closed because it's not an actionable thing right now since Jest is built on Jasmine and it would be hard to change, slash not even sure it is the right move. I think that's super valid, GitHub doesn't have a great solution for long-lived discussions, so sometimes maintainers need to close issues to maintain sanity 😄

@ovidiuch
Copy link

@ianstormtaylor did you ever find a solution to your use case in the end? I also want to generate tests based on a dynamic list of fixtures, which is obtained async, and I can't think of way to do it. It seems like Jest determines that there are 0 tests in my file by the time I have the async fixture list ready.

@Andarist
Copy link
Contributor

Andarist commented Oct 28, 2017

I've found a temporary solution for this just yesterday. At the moment I've wrapped my async script in executable and im calling it sync way in my tests:

#!/usr/bin/env node
const myAsyncScript = require('../myAsyncScript')
const args = process.argv.slice(2)

myAsyncScript(...args)
	.then(data => {
		process.stdout.write(JSON.stringify(data))
		process.exit()
	})
	.catch(() => {
		process.exit(1)
	})

index.test.js

import { execFileSync } from 'child_process'
const result = JSON.parse(execFileSync(require.resolve('../scripts/bin/pathToExecutable'), [...args]).toString())

describe('...', () => {
  for (const testCase of result) {
    test(testCase.title, () => {
      // ...
    })
  }
})

@gregorskii
Copy link

This seems like a useful feature, I think the ask is for the TESTS themselves to be defined by an asynchronous process. I have a similar use case where I need to run my Puppeteer tests against a set of different URLs, where the URLs are defined by calling a Google Sheet.

@exogen
Copy link

exogen commented Mar 11, 2018

This would be nice, and totally does make sense. There doesn't seem to be a way to asynchronously define the tests themselves at the moment – but if you're loading fixtures, that's exactly what you need to do.

In my case, I want Puppeteer to hit a styleguide page, then define a Jest test for each component in our styleguide that asserts that it visually matches (using jest-image-snapshot). At the moment the only solution is to only have one single giant test case.

@simonbuchan
Copy link

Another case of the above is wix/Detox#570 - getting a list of test devices to run tests on is async, you want to run your tests across each device:

describe('Attached', async () => {
  const devices = adb.devices();
  for (const { name } of devices) {
    describe(name, () => {
      beforeAll(async () => { detox.init(...); });
      afterAll(async () => { detox.cleanup(); });
      // as usual ...
    });
  }
});

Though of course that issue is about running them in parallel as well, which jest doesn't really have a reasonable way to do.

@sbekrin
Copy link

sbekrin commented Apr 5, 2018

As a workaround solution, Is it possible to enable nested test / it blocks by any chance? What should work in case of async resolution/retrieving of tests I believe.

@SimenB
Copy link
Member

SimenB commented Apr 14, 2018

See #5673 for a PR. We're not sure if we're gonna pull it in, but it's the closest we've gotten :)

@ulrichb
Copy link
Contributor

ulrichb commented Sep 13, 2018

+1 For an async describe-callback (to be able to pass async-generated data to the inner tests).

@cpojer Please consider reopening.

@lukepighetti
Copy link

Why I want async describes: I'd like to make a call to an API and run several tests on the output instead of making a call to the API for every single test. Is there a way to do this without an async describe?

@SimenB
Copy link
Member

SimenB commented Oct 2, 2018

let data;

beforeAll(async () => {
  data = await makeDataCall();
});

test('something', () => {
  expect(data).toBe(blablabla);
});

@timtrinidad
Copy link
Contributor

timtrinidad commented Oct 9, 2018

This would also be helpful for test.each. Unfortunately right now this throws an error:

let data;
beforeAll(async () => {
  data = await makeDataCall(); // returns test cases
});

describe('something', () => {
  test.each(data.cases)(...); // throws "cannot read property of undefined"
});

@atuttle
Copy link

atuttle commented Jan 4, 2019

I'm attempting to use Jest to enforce a set of rules on a set of configuration files. Since there could be N files, I'm attempting to enumerate them and create a test for each one individually, like this:

const getAllConfigs = require('../get-all-configs');

let configs;
beforeAll(async () => {
	configs = await getAllConfigs();
	return true;
});

describe('Validate auth config', () => {
	configs.forEach(conf => {
		test(`${conf} ~ auth config`, () => {
			const f = `../configs/${conf}.js`;
			const config = require(f);

			expect(config.environments.prod.auth).toBeDefined();
			expect(Array.isArray(config.environments.prod.auth)).toEqual(true);
			//...
		});
	});
});

At the configs.forEach this throws:

TypeError: Cannot read property 'forEach' of undefined

I see that my beforeAll callback can return a promise, but this error seems to indicate that Jest is not waiting for that promise to resolve before executing my describe().

If I change my describe() to test() (and comment out the inner test() definition but keep its assertions) then it works... But because we can't nest tests, I have to put all assertions across all files into one test, and I don't see any way to get extra context when something fails, so all I will know is that one of my config files fails the highlighted assertion, but not which one.

If I could nest tests, or if describe() weren't executed until after my async beforeAll() callback completed, then I could put the necessary context into the test name so that it's clear which file is breaking the config rules.

Aside from putting everything into one big test and using assert in place of expect (so that I can add a custom failure message) (unless there is a better way now? it's been a year since that comment), is there any other way to accomplish all of this?

@atuttle
Copy link

atuttle commented Jan 16, 2019

In my case I was able to work around this by using fs.readdirSync() and building my tests after getting the file list back.

@mattphillips
Copy link
Contributor

@atuttle the issue you're having is because tests need to be defined synchronously for Jest to be able to collect them, its the same issue we have internally with jest-each see the Troubleshooting docs

@Jezorko
Copy link

Jezorko commented Mar 15, 2019

Alternative workaround: define a testCaseWithAsyncRequirement function like:

function testCaseWithAsyncRequirement(requirementPromise, callback) {
    return async () => {
        let requirement;
        let requirementError;

        try {
            requirement = await requirementPromise;
        } catch (error) {
            console.error('condition not satisfied, skipping test');
            console.log(error);
            requirementError = error !== undefined ? error : new Error('undefined value thrown');
        }

        if (requirementError === undefined) {
            await callback(requirement);
        }
    };
}

and then use it in your test case:

const assert = require('assert');
const use = require('./TestUtils').testCaseWithAsyncRequirement;

describe('testCaseWithAsyncRequirement', () => {

    it('should be OK', use(Promise.resolve('ok'), async data => {
        assert.strictEqual('ok', data);
    }));

    it('should be skipped and not fail', use(Promise.reject(new Error("")), async data => {
        assert.fail(`this should never happen: ${data}`)
    }));

    it('should be skipped and not fail even though error was undefined', use(Promise.reject(undefined), async data => {
        assert.fail(`this should never happen: ${data}`)
    }));

    it('should fail', use(Promise.resolve('invalid'), async data => {
        assert.strictEqual('ok', data);
    }));

});

Enjoy (:

@anodynos
Copy link

@Jezorko I tried the above solution, but it doesn't work with an async beforeXxx to grab the value generated there, cause the use(promise, ...) call takes place before the beforeAll has executed. That's the whole issue I think.

@anodynos
Copy link

This issue is such a bummer :-(

@Jezorko
Copy link

Jezorko commented Apr 27, 2019

@anodynos can you show me an example code please?

@gregorskii
Copy link

Is this dead? Ran into another use case for this.

Request some async data and setup an it block per item in a json array.

:/

@thymikee
Copy link
Collaborator

thymikee commented Aug 3, 2019

You can do it in a beforeAll inside the describe

@gregorskii
Copy link

Unfortunately you cannot if the describe defines the it blocks in a loop. That’s the main issue... 🤷‍♀️

@onebrother21
Copy link

onebrother21 commented Sep 10, 2019

I believe I have the simplest use case to date. If one wants to generate a Node server (or in my case, a custom class that incorporates a Node server) then test with Supertest, you have to get the app as the first "it" in each test suite! I would love to be able to load my app & Supertest once (during the "describe") and pass the app to each test suite. I've tried "beforeAll" but as other have noted, Jest does not wait for the promise to execute before running nested describes.

desc("Run All Tests",() => {
  let o:any;
  beforeAll(async () => {o = await testServer();});
  it("init w/o errors",async () => {
    await is(o.master);
    await is(o.app);},99999); //test passes because of async/await in "it"
  baseApiTests(o); //should return nested test suite using fulfilled "o";
});

"TypeError: Cannot destructure property master of 'undefined' or 'null'"
"export const baseApiTests = ({master,app}:{master:AppMaster;app:any;}) => desc("API",() => {..."

because we need async describe :)

@loban
Copy link

loban commented Nov 6, 2019

Why was this closed? Is this feature "something that will never be done" or "an invalid request"? Otherwise, doesn't it make sense to keep it open to invite PRs or further discussion?

@jflatow
Copy link

jflatow commented Feb 3, 2020

This is clearly an in-demand feature, it has been popping up again and again going on 4 years now.

@owap
Copy link

owap commented Feb 10, 2020

Want to add my two cents for why I think this is a worthwhile feature:
I want to be able to dynamically set test timeouts (fetched asynchronously).

Here's how timeouts work now:

// Not dynamic; works fine:
const TIMEOUT = 1000; 

describe('foo', () => {
  test('bar', () => {
    // ...
  }, TIMEOUT);
});

The above does not meet my use case, because I want to be able to set the timeout dynamically.
Fetching the timeout in beforeAll does not seem to be an option, because a timeout variable would live in the scope of the describe block, and would be undefined when defining the test:

describe('foo', () => {
  let timeout;

  beforeAll(async () => {
    timeout = await getTimeout();
  });

  test('bar', () => {
    // ...
  }, timeout);  // Timeout is undefined here
});

If we allow for promises in describe blocks, we'd be able to use async/await for fetching our timeouts:

describe('foo', async () => {
  const timeout = await getTimeout();

  test('bar', () => {
    // ...
  }, timeout);  // Timeout is defined 🎉
});

Thanks everyone for your hard work on Jest, and please let me know if there's a better way to configure test timeouts dynamically on a per-test basis!

@owap
Copy link

owap commented Feb 10, 2020

Ahh, anybody looking for a workaround to ☝️ the above use case might be interested to learn about globalSetup, which I didn't know about before making the above post 😅

In this case, I'm ok fetching the timeout before the entire suite is run, so I just made one of these:

// globalSetup.js
import { getMaxTestDuration } from './some/file.js';

export default async function globalSetup() {
  process.maxTestDuration = await getMaxTestDuration();
}

Then, in my jest config:

globalSetup: './globalSetup.js',

Now the timeout is available to use in my tests, like:

describe('a', () => {
  test('b', () => {
    // ...
  }, process.maxTestDuration);
});

Perhaps you can front-load some async operations globally instead of on a per-describe-block basis? Worked for me!

@SimenB
Copy link
Member

SimenB commented Feb 12, 2020

Long term solution to this problem ("I need to do something async before declaring my tests") is probably top level await and using native ESM.

Former can be tracked in nodejs/node#30370 and the latter in #9430

@jflatow
Copy link

jflatow commented Feb 13, 2020

I mean not really, its tied to describe block. Allowing async describe is the best solution to avoid e.g. the example above: let o:any; beforeAll(async () => {o = await testServer();});

@BinaryShrub
Copy link

I feel like this would be valuable to break down tests to single expect statements for better organization. For example I want to make a supertest call in describes for POST /items and be able to have a test for statusCode 200, a test for the body, a test for the headers, etc , etc... rather than having to smash them all into one test or calling supertest for each.

@lobsterkatie
Copy link
Contributor

Just adding my $.02 that though in my case it can be worked around, having async describe would give me the nicest/cleanest way to accomplish what I want. So if you're tallying votes, count me on Team AsyncDescribe.

(In my case, I'm using a function which is basically the JS equivalent of a Python context manager, more or less like this:

describe("myClient", () => {
  const testClient = new MyClient(options);

  describe("upload", () => { ... }); 

  describe("download", async () => {
    await withTempDir(tempDir => {
      it("does what it should", async () => {
        await testClient.download('somefile', tempDir);
        expect(something).toHaveHappened;
        expect(somethingElse).not.toHaveHappened;
      });

      it("errors when appropriate", () => { ... });
    
      it("handles this edge case", () => { ... });
      
      it("handles this other edge case", () => { ... });

      // etc
    });
  }); 
}); 

I have to await withTempDir, or jest doesn't see my download tests at all. But then I need async on my describe callback... and that lands us back in this issue.)

@stanlaktionov
Copy link

Usefull feature for jest pupeteer. Especially for loading asyn data, that can be used inside descibe.each. I think it is worth to implement

@github-actions
Copy link

This issue 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 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests