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.

Sign up for GitHub

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

Jump to bottom

Running tests with RECORD=true leaves a non-clean working tree #855

Closed
1 task done
myitcv opened this issue Sep 21, 2023 · 1 comment · Fixed by #856
Closed
1 task done

Running tests with RECORD=true leaves a non-clean working tree #855

myitcv opened this issue Sep 21, 2023 · 1 comment · Fixed by #856
Labels
bug

Comments

@myitcv
Copy link
Contributor

myitcv commented Sep 21, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Running the lexer tests against def00e9 with RECORD=true leaves the git working tree in an non-clean state.

To Reproduce

Against commit def00e9:

$ RECORD=true go test ./lexers
ok      github.com/alecthomas/chroma/v2/lexers  1.448s
$ go test ./lexers
ok      github.com/alecthomas/chroma/v2/lexers  1.581s
$ git status
HEAD detached at origin/master
Changes not staged for commit:
        modified:   lexers/testdata/analysis/bash.expected
        modified:   lexers/testdata/analysis/c.ifdef.expected
        modified:   lexers/testdata/analysis/c.ifndef.expected
        modified:   lexers/testdata/analysis/c.include.expected
        modified:   lexers/testdata/analysis/cpp.include.expected
        modified:   lexers/testdata/analysis/cpp.namespace.expected

no changes added to commit

So whilst the lexer tests pass, updating the expectations files results in changes.

This seems less than ideal for people making contributions to the repo, especially if they are updating expectations in specific files because they could easily include unrelated changes in others, without a clear understanding of why there are differences.

According to my analysis there are two reasons behind this:

  • Newlines are not consistently being written to expectation files.
  • The comparison of expected vs actual happens after trimming of space and parsing of values, meaning different byte sequences can be considered equal.

I have a PR that includes a number of commits that propose a solution to this, which I will push up for discussion to complement this issue.
@myitcv myitcv added the bug label Sep 21, 2023
myitcv added a commit to myitcvforks/chroma that referenced this issue Sep 21, 2023
@myitcv
Assert lexer tests based on equality of byte encodings
fd4eb00 
This helps to ensure that running RECORD=true go test ./lexers on a
given commit for which the tests pass will not result in any changes in
the working tree.

Fixes alecthomas#855
myitcv added a commit to myitcvforks/chroma that referenced this issue Sep 21, 2023
@myitcv
Assert lexer tests based on equality of byte encodings
53e71f9 
This helps to ensure that running RECORD=true go test ./lexers on a
given commit for which the tests pass will not result in any changes in
the working tree.

Notes that as a result of this change, the following command results in
a clean working tree:

    RECORD=true go test ./lexers

Fixes alecthomas#855
@myitcv myitcv mentioned this issue Sep 21, 2023
Merged
@alecthomas
Copy link
Owner

alecthomas commented Sep 21, 2023 •
edited

Moved comment to PR.

alecthomas pushed a commit that referenced this issue Sep 21, 2023
@myitcv
lexers: move to comparing bytes in tests (#856)
ccd8d68 
* test: fix up writing up float scores with trailing newlines

This ensures we consistently write a trailing newline when RECORD=true
is set.

* lexers: update expected files using proper newlines

This is the mechanical result of running:

    RECORD=true go test ./lexers

* Assert lexer tests based on equality of byte encodings

This helps to ensure that running RECORD=true go test ./lexers on a
given commit for which the tests pass will not result in any changes in
the working tree.

Notes that as a result of this change, the following command results in
a clean working tree:

    RECORD=true go test ./lexers

Fixes #855
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

lexers: move to comparing bytes in tests myitcvforks/chroma
2 participants
@alecthomas @myitcv