Skip to content

Comments

Pretty print Diagnostics in snapshot tests#3906

Merged
MichaReiser merged 1 commit intomainfrom
test-snapshots
Apr 11, 2023
Merged

Pretty print Diagnostics in snapshot tests#3906
MichaReiser merged 1 commit intomainfrom
test-snapshots

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Apr 7, 2023 •
edited
Loading

This PR extends the TextEmitter to print diffs for fixes (explicit opt-in) and changes the snapshot tests to use the TextEmitter instead of assert_yaml_snapshot.

Pretty printing the diagnostics makes our linter tests more resilient to changes. For example, changing the field ordering of Diagnostic or its representation no longer breaks the snapshot tests because it doesn't change the pretty printed output.

The pretty printed output further has the advantage that it is easier to spot errors. No more offset counting to verify that the diagnostic or edit points to the right locations.

This was referenced Apr 7, 2023
Merged
Merged
Merged
Merged
Merged
Merged
@MichaReiser
Copy link
Member Author

MichaReiser commented Apr 7, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser force-pushed the test-snapshots branch 2 times, most recently from 1042404 to bc9565b Compare April 7, 2023 17:41
MichaReiser
MichaReiser commented Apr 7, 2023
View reviewed changes
...es/flake8_blind_except/snapshots/ruff__rules__flake8_blind_except__tests__BLE001_BLE.py.snap
57 | pass
|

./resources/test/fixtures/flake8_blind_except/BLE.py:60:8: BLE001 Do not catch blind exception: `Exception`
Copy link
Member Author

@MichaReiser MichaReiser Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

a blind exception... interesting 🙈

@MichaReiser MichaReiser force-pushed the test-snapshots branch 3 times, most recently from 625430d to c61412e Compare April 7, 2023 18:21
charliermarsh
charliermarsh reviewed Apr 7, 2023
View reviewed changes
crates/ruff/src/message/diff.rs

/// Renders a diff that shows the code fixes.
///
/// The implementation isn't fully fledged out and only used by tests. Before using in production, try
Copy link
Member

@charliermarsh charliermarsh Apr 7, 2023

Choose a reason for hiding this comment

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

What do we do in production? Continue to show just the changed line?

Copy link
Member

@charliermarsh charliermarsh Apr 7, 2023

Choose a reason for hiding this comment

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

Oh right, sorry, in production we don't show suggested fixes at all, we only show the suggestion message, and highlight the relevant code.

Copy link
Member

@charliermarsh charliermarsh Apr 7, 2023

Choose a reason for hiding this comment

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

I do wonder if there's anything we can leverage from Clippy or rustc to facilitate printing diffs.

Copy link
Member Author

@MichaReiser MichaReiser Apr 7, 2023

Choose a reason for hiding this comment

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

We can look into it when redoing Diagnostics. I would like to replace annotate snippet and colored. Ideally, the code frame and diff is something we build ourselves as it is an important core component of all our tools. This is something that we could inherit from Rome

charliermarsh
charliermarsh reviewed Apr 7, 2023
View reviewed changes
crates/ruff/src/message/diff.rs
}

impl<'a> Diff<'a> {
pub fn from_message(message: &'a Message) -> Option<Diff> {
Copy link
Member

@charliermarsh charliermarsh Apr 7, 2023

Choose a reason for hiding this comment

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

Should this be try_from_message? Or is try_from_* only used for Result?

Copy link
Member Author

@MichaReiser MichaReiser Apr 7, 2023

Choose a reason for hiding this comment

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

Not sure. NonZeroUsize returns Option from new. My reasoning is that it isn't failing, it's just that not all Messages can print a diff.

charliermarsh
charliermarsh reviewed Apr 7, 2023
View reviewed changes
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023 •
edited
Loading

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.01     14.3±0.02ms     2.9 MB/sec    1.00     14.1±0.03ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.6±0.01ms     4.7 MB/sec    1.00      3.6±0.00ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    458.9±1.56µs     6.4 MB/sec    1.00    457.8±0.81µs     6.4 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.1±0.03ms     4.2 MB/sec    1.00      6.0±0.05ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.02      7.3±0.01ms     5.5 MB/sec    1.00      7.2±0.01ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1637.4±8.55µs    10.2 MB/sec    1.00   1623.1±3.21µs    10.3 MB/sec
linter/default-rules/numpy/globals.py      1.00    180.4±0.34µs    16.4 MB/sec    1.00    180.4±1.29µs    16.4 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.4±0.00ms     7.6 MB/sec    1.00      3.3±0.01ms     7.7 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.01     16.1±0.31ms     2.5 MB/sec    1.00     16.0±0.27ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.2±0.15ms     4.0 MB/sec    1.00      4.1±0.10ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    498.7±8.04µs     5.9 MB/sec    1.01   503.7±13.83µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.8±0.16ms     3.7 MB/sec    1.00      6.8±0.15ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.2±0.13ms     5.0 MB/sec    1.00      8.2±0.12ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1789.4±34.77µs     9.3 MB/sec    1.01  1815.9±37.32µs     9.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    196.3±5.87µs    15.0 MB/sec    1.01    197.3±6.50µs    15.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.06ms     6.8 MB/sec    1.01      3.8±0.07ms     6.8 MB/sec

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Apr 11, 2023
@MichaReiser MichaReiser force-pushed the edit-operation branch 2 times, most recently from c7b5b1d to 2323617 Compare April 11, 2023 08:49
Base automatically changed from edit-operation to main April 11, 2023 08:56
@MichaReiser MichaReiser enabled auto-merge (squash) April 11, 2023 08:59
@MichaReiser MichaReiser merged commit e8aebee into main Apr 11, 2023
@MichaReiser MichaReiser deleted the test-snapshots branch April 11, 2023 09:03
This was referenced Apr 11, 2023
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

No one assigned

Labels

internal An internal refactor or improvement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichaReiser @charliermarsh