swc icon indicating copy to clipboard operation
swc copied to clipboard

feat(es/testing): Parse test code as a `Program` instead of a `Module`

Open leafypout opened this issue 1 year ago • 5 comments

Description: This PR addresses the issue described in #8713

BREAKING CHANGE: This will break existing unit tests that use fold_module/visit_module/visit_mut_module if the visitor is intended to work for both modules and scripts, instead of using fold_program/visit_program/visit_mut_program. This will also break existing unit tests if they're testing with input code that gets parsed as a script in parse_program if the visitor expects a module (they will need to update their test! calls to add module as the first argument, or use a function like apply_module_transform)

Related issue (if exists):

  • Closes #8713

leafypout avatar Jul 16 '24 23:07 leafypout

Several swc tests rely on the parsed test code being a module due to their visitors, going to have to fix these. The good news is that this will fix any of these issues for people using parse_script. e.g. some of the simplifier tests fail on scripts but not on modules.

leafypout avatar Jul 17 '24 00:07 leafypout

CodSpeed Performance Report

Merging #9264 will not alter performance

Comparing levi-nz:issue-8713 (df6db60) with main (8263da1)

Summary

✅ 194 untouched benchmarks

codspeed-hq[bot] avatar Jul 17 '24 01:07 codspeed-hq[bot]

I will probably clean up the internal API a bit because I think it's a bit messy, but essentially how testing works now:

  • Current test implementations will continue to work, unless the visitor(s) being tested is implemented wrongly. Since parse_program is the default now and not parse_module, any visitors using visit_module instead of visit_program won't work if the parsed AST ends up being a script (because visit_module won't be called, only visit_script will). Visitors should use visit_program if they want to handle both modules and scripts.
  • Callers can force the test implementation to use parse_module or parse_script instead of parse_program by calling test!(module, ...) and test!(script, ...) respectively.
  • Some tests need to use test!(module, ...) as mentioned above, since their visitors depend on the code being a module. A good example are the fixture tests in swc_ecma_transforms_module.

leafypout avatar Jul 17 '24 09:07 leafypout

Sounds good, thank you!

kdy1 avatar Jul 17 '24 09:07 kdy1

Not sure what we think of the internal api, test!(module, ...), test!(script, ...), apply_module_transform etc.

leafypout avatar Jul 18 '24 02:07 leafypout

🦋 Changeset detected

Latest commit: df6db609bb50a6ad3e0cb24c091001d742f7f686

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Oct 07 '24 08:10 changeset-bot[bot]

@kdy1 can you un-merge? Not finished yet. Forgot to mark as draft

leafypout avatar Oct 08 '24 02:10 leafypout

I've pushed my changes (should be good to go now), but I'll need to rebase once you revert the commit.

leafypout avatar Oct 08 '24 03:10 leafypout

@levi-nz Done. https://github.com/swc-project/swc/pull/9621

kdy1 avatar Oct 08 '24 03:10 kdy1