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

test: use cmp.Diff for comparing output #558

Merged
merged 2 commits into from
Sep 27, 2023
Merged

test: use cmp.Diff for comparing output #558

merged 2 commits into from
Sep 27, 2023

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Sep 27, 2023

This makes it easier to determine what is wrong in CLI tests by showing an actual diff; while in most cases this is an improvement, I have found it's sometimes useful to have the raw output printed so I've included an env variable to allow switching easily with cmp.Diff being the default.

In order to reduce the diff noise when a test does fail, this also switches to replacing occurrences of the current working directory in the actual output with <rootdir>; this also means that the output will be what should be in the test cases, rather than the absolute path that people would have to replace with <rootdir>.

While this could be used throughout the whole test suite, I've just applied this to the CLI tests for now because I think they've got the most to gain whereas it'd be a lot of tedious work to switch to using it everywhere; it should be easy to switch to using it in other places over time.

I have also confirmed that dedent correctly handles both spaces and tabs - you can mix and match them without issue (and in fact a few of the tests are using spaces instead of tabs).

@codecov-commenter
Copy link

Codecov Report

Merging #558 (0486230) into main (5862bfd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #558   +/-   ##
=======================================
  Coverage   78.45%   78.45%           
=======================================
  Files          77       77           
  Lines        5273     5273           
=======================================
  Hits         4137     4137           
  Misses        971      971           
  Partials      165      165           

@another-rex
Copy link
Collaborator

I think the dedent assumes you're tab sizing of 2 (it replaces tab with 2 spaces), but I think we often have a tab sizing of 4, which is where we run into the issue

@G-Rath
Copy link
Collaborator Author

G-Rath commented Sep 27, 2023

Right I see - it is possible to have a single space but everything ends up still being aligned; I don't think there is an easy way to handle that - maybe you could check if a line includes any spaces at the start of the string and error? but that might still cause issues in some tests. Either way I don't want to try tackling that in this PR.

Also note that the indent size is set to 2 via .editorconfig so people should technically be using a tab size of 2.

@another-rex another-rex merged commit 8113801 into google:main Sep 27, 2023
7 checks passed
@G-Rath G-Rath deleted the use-cmp branch September 27, 2023 02:00
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.

None yet

3 participants