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

[Suggestion] Add test scaffolding #502

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Suggestion] Add test scaffolding #502

wants to merge 1 commit into from

Conversation

drwpow
Copy link

@drwpow drwpow commented Feb 8, 2025

Summary

This is an optional PR! It does not solve an existing issue but may make contribution easier. Please review and accept or reject at your discretion.

Reasoning

In investigating adding support for #448, I noticed there weren’t many build tests. I didn’t have an easy way to extend the existing tests to first see if a certain config generated a certain output. So I started expanding the tests so that one could easily tweak a single build option and test its output.

This snapshots files which IMO is the best solution for build tools like this for the following reasons:

  • Build tools like unbuild MUST make certain guarantees to purity. Meaning, as long as the inputs don’t change, the output shouldn’t either. Thus, snapshots shouldn’t flake, in theory (given tightly-scoped inputs, which these tests have)
  • When it comes to tests that test complex build systems like this, manual assertions are difficult to impossible to write. Think about all the config options, all the files generated, every little nuance of everything (how would we test that all sourcemap comments in all files are correct?)—there are so many scenarios that are easy to miss. But with snapshots, new tests get 100% coverage automatically with no work.
  • Because any potential output change may be a breaking change, snapshots guard against accidental bugs. Erring on any change in output is desirable because it is likely a regression.
  • The downside to snapshots is they only test changes, not desired behavior and for that reason are not suitable for all scenarios. But I would like to argue that they are fantastic for build systems where every change MAY be a regression and surfacing any change as a potential error for inspection is desirable

Reviewing

Mostly would like feedback on:

  • Would this setup improve coverage for existing build options?
  • Would this test setup be easy to expand on (especially for contributors like myself)?
  • Anything additional that we’d want to test?

@drwpow drwpow force-pushed the tests branch 3 times, most recently from 08c12a3 to d8e57fb Compare February 8, 2025 00:39
"
`;

exports[`Build fixtures > sourcemap (mkdist) > index.mjs 1`] = `
Copy link
Author

Choose a reason for hiding this comment

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

Note: this test revealed that mkdist doesn’t support sourcemap: true. Should it?

I figured that this test is just a recording of the current behavior (and can surface other bugs like this!)


it.each(tests)("%s", async (_, { dir, ...buildOptions }) => {
const cwd = new URL(dir.replace(/\/?$/, "/"), import.meta.url);
await build(fileURLToPath(cwd), false, buildOptions);
Copy link
Author

Choose a reason for hiding this comment

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

Note: the tests are set up to allow any build options if needed, even though this currently doesn’t take use of that. But we could test stubs, etc. if needed! (Should we add more tests for those?)

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.

1 participant