-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(es/testing): Parse test code as a Program
instead of a Module
#9264
Conversation
Program
instead of a Module
Program
instead of a Module
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 |
CodSpeed Performance ReportMerging #9264 will not alter performanceComparing Summary
|
I will probably clean up the internal API a bit because I think it's a bit messy, but essentially how testing works now:
|
Sounds good, thank you! |
Not sure what we think of the internal api, |
@@ -10,7 +10,7 @@ use swc_common::{chain, comments::SingleThreadedComments, Mark}; | |||
use swc_ecma_parser::{EsSyntax, Syntax, TsSyntax}; | |||
use swc_ecma_transforms_base::{assumptions::Assumptions, resolver}; | |||
use swc_ecma_transforms_proposal::{decorator_2022_03::decorator_2022_03, DecoratorVersion}; | |||
use swc_ecma_transforms_testing::{test_fixture, FixtureTestConfig}; | |||
use swc_ecma_transforms_testing::{test_module_fixture, FixtureTestConfig}; |
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.
Can we have FixtureTestConfig.module: Option<bool>
, which defaults to None
, and
Some(true)
: Module (and applies visitor after wrapping as Program)Some(false)
: Script (and applies visitor after wrapping as Program)None
: Program
@@ -96,6 +96,7 @@ fn transformation(t: &Tester) -> impl Fold { | |||
|
|||
// transformation_declaration | |||
test!( | |||
module, |
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 love this idea 👍
🦋 Changeset detectedLatest commit: df6db60 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 |
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.
Thank you!
@kdy1 can you un-merge? Not finished yet. Forgot to mark as draft |
I've pushed my changes (should be good to go now), but I'll need to rebase once you revert the commit. |
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 usingfold_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 inparse_program
if the visitor expects a module (they will need to update theirtest!
calls to addmodule
as the first argument, or use a function likeapply_module_transform
)Related issue (if exists):
test_transform
should probably useparse_program
instead ofparse_module
#8713