Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

test(rome_js_formatter): Add prettier's JSX tests #3255

Merged
merged 2 commits into from
Sep 20, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Sep 19, 2022

Summary

This PR extracts the JSX tests from prettier and updates existing tests.

There are a few instability issues, most related to comments. I'll look into those as part of the comments refactoring

Test Plan

Verified that the new snapshots run as part of cargo test

Average compatibility: 86.15 -> 86.16
Compatible lines: 84.93 -> 84.99

This PR extracts the JSX tests from prettier and updates existing tests.

There are a few instability issues, most related to comments. I'll look into those as part of the comments refactoring
This PR extracts the JSX tests from prettier and updates existing tests.

There are a few instability issues, most related to comments. I'll look into those as part of the comments refactoring
@MichaReiser MichaReiser added the A-Formatter Area: formatter label Sep 19, 2022
@netlify
Copy link

netlify bot commented Sep 19, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 380a55b
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/632886af74ac4100087e1bc5

@MichaReiser MichaReiser temporarily deployed to netlify-playground September 19, 2022 15:11 Inactive
@MichaReiser MichaReiser changed the title test(rome_js_formatter): Add prettier JSX tests test(rome_js_formatter): Add prettier's JSX tests Sep 19, 2022
@github-actions
Copy link

@MichaReiser
Copy link
Contributor Author

I'll handle the comments issue as part of the #3227

@ematipico
Copy link
Contributor

What's the compatibility coverage now?

@MichaReiser
Copy link
Contributor Author

What's the compatibility coverage now?

I added them to the test plan. They aren't changing much (and I prefer to fix any regressions as part of dedicated PRs)

@@ -0,0 +1,6 @@
// https://github.com/typescript-eslint/typescript-eslint/pull/4382
function decorator() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember what we do with skipped/unsupported syntax. Do we include them in the test suite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are included but "excluded" in the report by testing on the test file name. We don't ignore decorator tests for now

@@ -0,0 +1,18 @@
<style jsx>{`
Copy link
Contributor

@ematipico ematipico Sep 20, 2022

Choose a reason for hiding this comment

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

What should we do with this test? It would tarnish our coverage, we don't support framework/library specific formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We keep them for now. We only excluded tests where the parser fails to parse the syntax (because it's unlikely that the formatter can then produce any reasonable result)

@MichaReiser MichaReiser merged commit 9a767d6 into main Sep 20, 2022
@MichaReiser MichaReiser deleted the test/prettier-jsx-tests branch September 20, 2022 09:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants