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

Distribute integration test to single test files #455

Merged

Conversation

devjiwonchoi
Copy link
Contributor

@devjiwonchoi devjiwonchoi commented Feb 18, 2024

This PR initiated a migration of single-file integration tests to each have their own test files.

  • Moved utility functions like executeBunchee from cli/utils to testing-utils
  • Added filesToRemove param to be received on createTest to remove files after the tests like tsconfig.json.

The migration range of this PR covers:

test/integration/basic-jsx

// ...

test/integration/js-only

Resolves #449

@huozhi
Copy link
Owner

huozhi commented Feb 18, 2024

We could ship part of them and migrate the rest in the follow PRs! Just for reduce the pressure of reviewing 🤣

// Additional files to remove inside the fixtures directory
// e.g. tsconfig.json
if (filesToRemove) {
for (const file of filesToRemove) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added filesToRemove to createTest to remove additional files created during tests (e.g. tsconfig.json)

Copy link
Owner

Choose a reason for hiding this comment

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

Is that possible to do it in beforeEach or beforeAll in the actual test? That's also why I want to refactor the tests to have less options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Will remove it thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't mind, could you tell me why you're not using temporary dirs?

Copy link
Owner

Choose a reason for hiding this comment

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

I want to do e2e at the begining, and linked bunchee there, so it can be seaprate from the installed packages in current project. but it's a long term refactor work

@devjiwonchoi devjiwonchoi marked this pull request as ready for review February 18, 2024 14:14
test/testing-utils.ts Outdated Show resolved Hide resolved
@@ -21,6 +20,8 @@ describe('integration', () => {
'#!/usr/bin/env node',
)
}

await deleteFile(`${__dirname}/fixtures/tsconfig.json`)
Copy link
Owner

Choose a reason for hiding this comment

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

Kinda forgot the reason why we need to remove them here?

I though if we have 2 integration tests share the same directory, any required to delete files can be remove in the beforeEach/afterEach jest lifecycle.

But not sure if we still need them here for single test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, for a single test, absolutely not.

Copy link
Owner

@huozhi huozhi left a comment

Choose a reason for hiding this comment

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

Thanks!

import { readFile } from 'fs/promises'
import { createIntegrationTest, deleteFile, existsFile } from '../../utils'

afterEach(async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

we probably don't need it here, for single test

await deleteFile(`${__dirname}/fixtures/tsconfig.json`)
})

describe('integration', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

better to use different names, like "integration - cts" for describe() name, then give it("should ..") to describe the expectation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Do you want me to resolve it on this PR or other?

Copy link
Owner

Choose a reason for hiding this comment

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

Ye not blocking, can follow up later!

@huozhi huozhi merged commit bbfa097 into huozhi:main Feb 18, 2024
3 checks passed
@devjiwonchoi devjiwonchoi deleted the migrate-integration-test-to-single-test branch February 18, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate integration tests to have single test file in each folder
2 participants