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

[tests] A better sandboxing of tests and builds #123

Open
wants to merge 6 commits into
base: yoann/add-more-injection-points
Choose a base branch
from

Conversation

yoannmoinet
Copy link
Member

What and why?

After #121 it proved to be super difficult not to leak in between builds.

We need to better sandbox our fixture projects and our builds from each others to avoid any leakage in between.

How?

  • Some inconsequential configuration updates.
  • Better environment setup for Jest, where we can reliably instrument some flags
    • --cleanup=0 to prevent deletion of the temporary directories where builds happen.
    • --bundlers=webpack4,... to only build for a specific bundler.
    • --build to force build the plugins before running the tests.
  • Fixture projects are now isolated from the rest and are their own project (from yarn's perspective).
  • Each time we trigger a build within a test (using runBundlers()) it will create a new temporary directory, copy the fixture projects in it, and run the build in isolation.

@yoannmoinet yoannmoinet marked this pull request as ready for review January 16, 2025 10:24
@yoannmoinet yoannmoinet requested a review from a team as a code owner January 16, 2025 10:24
@yoannmoinet yoannmoinet requested review from nchapma2 and elbywan and removed request for a team and nchapma2 January 16, 2025 10:24
.node-version Outdated
@@ -1 +1 @@
18.19.0
18.20.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we still need this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably not.

`Invalid "${red(`--bundlers ${REQUESTED_BUNDLERS.join(',')}`)}".\nValid bundlers are ${FULL_NAME_BUNDLERS.map(
(b) => green(b),
)
.sort()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you sorting this static array? If you want it sorted, shouldn't the base static array be already pre-sorted?

Copy link
Member Author

Choose a reason for hiding this comment

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

absolutely not sure why I did this.

return path.resolve(workingDir, `./dist/${folderName}`);
};

const FIXTURE_DIR = path.resolve(__dirname, '../fixtures');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a blocker, but __dirname doesn't exist in ESM, only in CJS. Idk if this can create issues or not

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update once we transition to esm.

@@ -315,4 +322,4 @@ export const filterOutParticularities = (input: File) =>
// Exclude webpack buildin modules, which are webpack internal dependencies.
!input.filepath.includes('webpack4/buildin') &&
// Exclude webpack's fake entry point.
!input.filepath.includes('fixtures/project/empty.js');
!input.filepath.includes('fixtures/empty.js');
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer .endsWith to .includes, to avoid something like fixtures/empty.js/hello.js

Copy link
Member Author

Choose a reason for hiding this comment

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

fair.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 actually, given the context, I think we might also get fixtures/empty.js?something since it's within the bundling.
I've seen weird shit with the commonjs plugins for instance 😄
I prefer to filter out any mention of this path, which is also pretty much well under control.

@@ -33,7 +33,7 @@ jobs:
- name: Install node
uses: actions/setup-node@v4
with:
node-version: '18.19.0'
node-version: '18.x'
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, the setup-node action can be configured to use the exact same version as volta.

Could be handy!

Copy link
Member Author

Choose a reason for hiding this comment

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

super useful, thanks!

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.

3 participants