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

Use oxc's own graphical reporter for test snapshots #2241

Conversation

maurice
Copy link
Contributor

@maurice maurice commented Jan 31, 2024

Fixes #2217

Previously we were using the miette default, but given ours is a fork, and for example now prints slightly more relevant line- and column-numbers vs miette, we should dog-food our own have the tests tell us if/when the output changes.

I did actually scan all the snapshot deltas and all look correct to me.

One funny one I noticed was this

diff --git a/crates/oxc_linter/src/snapshots/no_empty_file.snap b/crates/oxc_linter/src/snapshots/no_empty_file.snap
index cfc53e1c..6f001fbd 100644
--- a/crates/oxc_linter/src/snapshots/no_empty_file.snap
+++ b/crates/oxc_linter/src/snapshots/no_empty_file.snap
@@ -2,6 +2,7 @@
 source: crates/oxc_linter/src/tester.rs
 expression: no_empty_file
 ---
+
   ⚠ eslint-plugin-unicorn(no-empty-file): Empty files are not allowed.
    ╭─[no_empty_file.tsx:1:1]
    ╰────
@@ -29,7 +30,7 @@ expression: no_empty_file
   help: Delete this file or add some code to it.
 
   ⚠ eslint-plugin-unicorn(no-empty-file): Empty files are not allowed.
-   ╭─[no_empty_file.tsx:1:1]
+   ╭─[no_empty_file.tsx:0:1]
  0 │ 
    · ▲
    ╰────
@@ -149,4 +150,3 @@ expression: no_empty_file
    ╰────
   help: Delete this file or add some code to it.
 
-

...which I suppose is technically correct but also a bit confusing perhaps? Should we make the line minimum 1? If so I can create another PR for that.

There is a subtle change in whitespace too - each file gains a newline at the start but looses one at the end. My assumption is that oxc's reporter is adding a newline at the start of each report (compared to miette's), plus I removed the extra newline in tester.rs or else the snapshot diffs would have been even larger.

Finally there are no changes to reports with multi-line annotations like this:

  ⚠ typescript-eslint(ban-types): Prefer explicitly define the object shape
   ╭─[ban_types.tsx:1:1]
 1 │ ╭─▶ const emptyObj: {
 2 │ │   
 3 │ ╰─▶         } = {foo: "bar"};
   ╰────
  help: This type means "any non-nullish value", which is slightly better than 'unknown', but it's still a broad type

Again I can create a separate PR to improve those and we should see snapshot diffs when I make that change 😄

I'd appreciate a quick review on this one if at all possible, given the high chance of conflict.

Previously we were using ihe miette default, but given ours is a fork, and for example now prints slightly more relevant line- and column-numbers vs miette, we should dog-food our own have the tests tell us if/when the output changes.
@github-actions github-actions bot added the A-linter Area - Linter label Jan 31, 2024
Copy link

codspeed-hq bot commented Jan 31, 2024

CodSpeed Performance Report

Merging #2241 will not alter performance

Comparing maurice:fix/use-oxc-graphical-reporter-for-snapshots (730b23e) with main (622a2c3)

Summary

✅ 17 untouched benchmarks

@maurice
Copy link
Contributor Author

maurice commented Jan 31, 2024

Finally there are no changes to reports with multi-line annotations like this:

  ⚠ typescript-eslint(ban-types): Prefer explicitly define the object shape
   ╭─[ban_types.tsx:1:1]
 1 │ ╭─▶ const emptyObj: {
 2 │ │   
 3 │ ╰─▶         } = {foo: "bar"};
   ╰────
  help: This type means "any non-nullish value", which is slightly better than 'unknown', but it's still a broad type

Again I can create a separate PR to improve those and we should see snapshot diffs

See #2242

@Boshen
Copy link
Member

Boshen commented Feb 1, 2024

This is much better!! Thank you so much!

@Boshen Boshen merged commit 9bbdec1 into oxc-project:main Feb 1, 2024
19 checks passed
Boshen pushed a commit that referenced this pull request Feb 1, 2024
Improves diagnostic report source line- and column-number for
**multiline annotations**

Requires #2241
@maurice maurice deleted the fix/use-oxc-graphical-reporter-for-snapshots branch February 1, 2024 07:47
IWANABETHATGUY pushed a commit to IWANABETHATGUY/oxc that referenced this pull request May 29, 2024
Fixes oxc-project#2217

Previously we were using the miette default, but given ours is a fork,
and for example now prints slightly more relevant line- and
column-numbers vs miette, we should dog-food our own have the tests tell
us if/when the output changes.

I did actually scan all the snapshot deltas and all look correct to me.

One funny one I noticed was this

```diff
diff --git a/crates/oxc_linter/src/snapshots/no_empty_file.snap b/crates/oxc_linter/src/snapshots/no_empty_file.snap
index cfc53e1c..6f001fbd 100644
--- a/crates/oxc_linter/src/snapshots/no_empty_file.snap
+++ b/crates/oxc_linter/src/snapshots/no_empty_file.snap
@@ -2,6 +2,7 @@
 source: crates/oxc_linter/src/tester.rs
 expression: no_empty_file
 ---
+
   ⚠ eslint-plugin-unicorn(no-empty-file): Empty files are not allowed.
    ╭─[no_empty_file.tsx:1:1]
    ╰────
@@ -29,7 +30,7 @@ expression: no_empty_file
   help: Delete this file or add some code to it.
 
   ⚠ eslint-plugin-unicorn(no-empty-file): Empty files are not allowed.
-   ╭─[no_empty_file.tsx:1:1]
+   ╭─[no_empty_file.tsx:0:1]
  0 │ 
    · ▲
    ╰────
@@ -149,4 +150,3 @@ expression: no_empty_file
    ╰────
   help: Delete this file or add some code to it.
 
-
```

...which I suppose is technically correct but also a bit confusing
perhaps? Should we make the line **minimum 1**? If so I can create
another PR for that.

There is a subtle change in whitespace too - each file gains a newline
at the start but looses one at the end. My assumption is that oxc's
reporter is adding a newline at the start of each report (compared to
miette's), plus I removed the extra newline in `tester.rs` or else the
snapshot diffs would have been even larger.

Finally there are no changes to reports with *multi-line* annotations
like this:

```
  ⚠ typescript-eslint(ban-types): Prefer explicitly define the object shape
   ╭─[ban_types.tsx:1:1]
 1 │ ╭─▶ const emptyObj: {
 2 │ │   
 3 │ ╰─▶         } = {foo: "bar"};
   ╰────
  help: This type means "any non-nullish value", which is slightly better than 'unknown', but it's still a broad type
```

Again I can create a separate PR to improve those and we should see
snapshot diffs when I make that change 😄

I'd appreciate a quick review on this one if at all possible, given the
high chance of conflict.
IWANABETHATGUY pushed a commit to IWANABETHATGUY/oxc that referenced this pull request May 29, 2024
…ect#2242)

Improves diagnostic report source line- and column-number for
**multiline annotations**

Requires oxc-project#2241
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor(linter): use the same miette graphical reporter for tests
2 participants