Skip to content

Unskip JS emit tests #1211

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

Merged
merged 11 commits into from
Jun 19, 2025
Merged

Unskip JS emit tests #1211

merged 11 commits into from
Jun 19, 2025

Conversation

jakebailey
Copy link
Member

After #1189, we no longer fall over in JS code. Unskip all of these tests so we can start working on them and catch any crashes.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR re-enables a suite of previously skipped JavaScript emit tests following #1189, ensuring that the compiler no longer crashes on valid JS constructs. The key changes include removal of AllowJS test skipping, updates to baseline outputs reflecting modern syntax (e.g. const, arrow functions, object shorthand), and adjustments in emitted declaration files reflecting updated export styles.

  • Removed the skip condition for AllowJS in the baseline runner.
  • Updated several test baselines to match the expected output from the compiler.
  • Modernized code patterns in test baselines (e.g. conversion from var to const, use of arrow functions).

Reviewed Changes

Copilot reviewed 835 out of 835 changed files in this pull request and generated no comments.

Show a summary per file
File Description
testdata/baselines/reference/submodule/compiler/argumentsReferenceInConstructor2_Js.js Adds baseline test for a constructor assigning to this["arguments"].
testdata/baselines/reference/submodule/compiler/argumentsReferenceInConstructor1_Js.js.diff Removes an unnecessary type declaration for the property in the diff output.
testdata/baselines/reference/submodule/compiler/argumentsReferenceInConstructor1_Js.js Baseline output for a constructor using the property "arguments".
testdata/baselines/reference/submodule/compiler/argumentsPropertyNameInJsMode2.js.diff Adjusts the declaration signature by removing rest parameters in the baseline diff.
testdata/baselines/reference/submodule/compiler/argumentsPropertyNameInJsMode2.js Updates the baseline output for function argument emission.
testdata/baselines/reference/submodule/compiler/argumentsPropertyNameInJsMode1.js.diff Modernizes the code with const and arrow functions, and updates export constructs.
testdata/baselines/reference/submodule/compiler/argumentsPropertyNameInJsMode1.js Updates baseline output to use concise ES6 syntax and object shorthand.
testdata/baselines/reference/submodule/compiler/amdLikeInputDeclarationEmit.js.diff Changes module export from export = ExtendedClass to an empty export; reflecting updated AMD handling.
testdata/baselines/reference/submodule/compiler/amdLikeInputDeclarationEmit.js Updates baseline for module export declaration and corresponding export handling.
testdata/baselines/reference/submodule/compiler/ambientRequireFunction(module=preserve).js.diff Replaces var with const for modern require usage in preserved module mode.
testdata/baselines/reference/submodule/compiler/ambientRequireFunction(module=preserve).js Reflects the updated const usage for require in the baseline output.
testdata/baselines/reference/submodule/compiler/ambientRequireFunction(module=commonjs).js.diff Modernizes require usage (var → const) for commonjs module output.
testdata/baselines/reference/submodule/compiler/ambientRequireFunction(module=commonjs).js Updates baseline output for commonjs modules with modern syntax.
testdata/baselines/reference/submodule/compiler/allowJscheckJsTypeParameterNoCrash.js.diff Modernizes code by replacing var with const and switching to shorthand method definitions.
testdata/baselines/reference/submodule/compiler/allowJscheckJsTypeParameterNoCrash.js Updates baseline output for improved JS type handling with modern syntax.
testdata/baselines/reference/submodule/compiler/accessorDeclarationEmitJs.js.diff Replaces namespace exports with explicit constant object declarations in the d.ts output.
testdata/baselines/reference/submodule/compiler/accessorDeclarationEmitJs.js Updates JS and d.ts baseline outputs for accessor declarations using modern export style.
testdata/baselines/reference/conformance/typeTagForMultipleVariableDeclarations.js Updates baseline output for type tag declarations in JS with multiple variables.
testdata/baselines/reference/conformance/deepElementAccessExpressionInJS.js Provides updated baseline output for deep element access expressions in JS.
internal/testutil/tsbaseline/js_emit_baseline.go Removes the AllowJS skip check to enable these tests, reflecting the updated compiler behavior.

@jakebailey jakebailey marked this pull request as draft June 17, 2025 18:43
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

For sure a lot of these diffs need to be looked at, but if we want some baselines to start with, this can be it.

@jakebailey
Copy link
Member Author

Yeah, I want to look at this a little harder before, because it's failing race checks due to emit ordering problems, which is I think must be fixed before we are able to merge it.

@jakebailey jakebailey marked this pull request as ready for review June 19, 2025 04:23
@jakebailey
Copy link
Member Author

Okay, I triaged the two failures (and cleaned up skipped tests a little bit). Theoretically this should pass CI.

@jakebailey jakebailey requested a review from weswigham June 19, 2025 04:23
@jakebailey jakebailey added this pull request to the merge queue Jun 19, 2025
Merged via the queue into main with commit 8073250 Jun 19, 2025
22 checks passed
@jakebailey jakebailey deleted the jabaile/unskip-js branch June 19, 2025 19:03
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.

2 participants