-
Notifications
You must be signed in to change notification settings - Fork 28
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
test: separate tests #424
test: separate tests #424
Conversation
Is this the change we want? I can do more work if this is expected. |
Thanks for the PR. The expectation is each test will have its own test file like |
b185004
to
ca114e3
Compare
I separated cli tests into multiple folders. If the same fixture is used, I put the test cases together in a Request CR, thanks! |
I was thinking to get a helper that automatically create a test directory for each test, and then use the fixed input filename, in each folder. So each test you only need to call the helper, and pass the directory (maybe just // we have a generic helper `createTest`, that can define what does the executor do
const createCliTest = createTest({
run(input, args) {
// do cli work
executeBunchee(...)
}
})
// in each test, `createCliTest` can return the result
it(...) {
const { code, stdout, distFiles, ... } = createCliTest({ directory: __dirname, args: [...] })
// test stdout etc
} |
ca114e3
to
242eb4e
Compare
|
||
expect(code).toBe(0) | ||
|
||
await removeDirectory(distDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could remove removeDirectory(distDir)
, because we create dist
in fixtures
instead of createTempDir
463864d
to
c32f6c7
Compare
There're code conflicts |
c32f6c7
to
3410dc4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nnecec The purpose to have cli and integration tests's cli separate is:
Integration tests shouldn't need to specify a folder, it should be current __dirname
by default always, no args. If we have one test file for each folder, case, instead group some of them in fixtures.
- integration/<xxx>
|- src/...
|- package.json
|- index.test.js
Cli tests can have flexible args, but if the input.js
filename is a fixed convention we don't need to do it.
I didn't expect we gonna change integration as well, as the early draft is only about cli. This is kinda a pretty big change now. If would be great if we can land the cli tests changes first, since they're not too much.
Then we get a new PR to change one integration test as an example, if it looks good we can migrate rest of them. It doesn't have to be a one time all migrated change.
Thank you!
3410dc4
to
f0ebf74
Compare
expect(fs.existsSync(typeFile)).toBe(true) | ||
expect(code).toBe(0) | ||
|
||
await removeDirectory(distDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done in afterEach/afterAll
Thanks for the PR, let's tackle the improves in the separate PR, will merge this for now |
resolve #389