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

Robustly clean up and restore state after tests #834

Open
nfischer opened this issue Apr 6, 2018 · 6 comments
Open

Robustly clean up and restore state after tests #834

nfischer opened this issue Apr 6, 2018 · 6 comments
Assignees
Labels

Comments

@nfischer
Copy link
Member

nfischer commented Apr 6, 2018

Inspired by #833. We should create a robust solution to ensure each test runs with fresh state, and we successfully cleanup after each test.

Right now, some tests can cd() to change the working directory, and we won't properly cleanup after them. As an example, here's a bad afterEach() which assumes the test doesn't change working directory:

shelljs/test/ln.js

Lines 18 to 20 in 9035b27

test.afterEach.always(t => {
shell.rm('-rf', t.context.tmp);
});

Another issue is that t.context.tmp usually refers to a relative path returned by utils.getTempDir(). I think we can be more robust by returning an absolute path.


We should create utility functions to do robust setup and teardown, to be used by every test suite (and the test suites can extend this to provide additional setup).

@freitagbr any additional thoughts?

@freitagbr
Copy link
Contributor

I agree that t.context.tmp should contain the full path of the temporary directory, not the relative. But, the setup and teardown functions should take care of setting all of this up. The setup and teardown functions might look like the beforeEach and afterEach in test/ln.js, for example. Then, in every test, we could just put test.beforeEach(setup) and test.afterEach(teardown). I don't know if ava has a way to do this globally for every test.

Another improvement might be using a dedicated temp directory module like tmp or tempy, though our solution should continue to work fine.

@nfischer
Copy link
Member Author

I don't know if ava has a way to do this globally for every test.

@freitagbr Huh? beforeEach() does run before every test, as-is. What did you mean?

@nfischer nfischer self-assigned this Apr 26, 2018
@nfischer
Copy link
Member Author

I'll write up a PR in the next couple days.

@freitagbr
Copy link
Contributor

Huh? beforeEach() does run before every test, as-is.

beforeEach() runs before every test in that file, but not for every file. Some test frameworks have a way to specify a beforeEach for every test in every file, but from reading the documentation, I don't think ava has a way to do this. But that's fine, it's not a big deal if each test file has beforeEach in it.

@nfischer
Copy link
Member Author

nfischer commented May 2, 2018

But that's fine, it's not a big deal if each test file has beforeEach in it.

Some test suites have different requirements from others. For example, some require a full copy of resources/ while others require an empty directory. So, for now it's nice that we have this flexibility.

Long-term, we should migrate most suites to the same beforeEach step (i.e. with a full copy of resources/). But even this doesn't make sense for tests like common.js, so we'll always want to specify per-suite.

@nfischer
Copy link
Member Author

Another thing (caught this in #874), we should robustly set process.env to its original value (resetting each property). This should happen in afterEach.always.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants