Use the minimal number of escapes when updating tests #28
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
|
Yeah, we intentionally didn't pick the simplest literal -- the benefit of using
|
...and add some serialization methods.
ecstatic-morse
force-pushed
the
minimal-escapes
branch
from
8e8e2a8 to
7bc8365
Compare
ecstatic-morse
force-pushed
the
minimal-escapes
branch
from
7bc8365 to
8a355f4
Compare
|
Updated version uses |
|
Build succeeded: |
bors
added a commit
to rust-lang/chalk
that referenced
this pull request
Feb 13, 2022
Allow tests to be updated automatically Uses the `expect-test` crate (thanks for the recommendation `@lnicola!)` to implement snapshot testing. `expect-test` is a bit more flexible than `insta`, so it seems like a good fit. This gives nicer diffs as well, since we don't strip whitespace prior to the comparison. Setting `UPDATE_EXPECT=1` before running `cargo test` will update all tests automatically. ## Blockers - [x] Omit empty lists from `Solution`'s `Display` impl. * `chalk` currently compares prefixes to make things less noisy, but that seems error prone anyways. * Harder than it seems at first, since you need to unintern to check whether a list is empty. - [x] Handle recursive/SLG comparisons better. * Many tests use `yields`, which compares the results of the two solvers against a known output. Unfortunately, the test macro will duplicate the `expect` invocation, so if neither output matches the expected one, `expect-test` will overwrite it *twice*, almost assuredly corrupting the test file. * There's many ways to fix this. ~~I've hacked around it here by adding a method to `Expect` (which is not yet upstream).~~ Most robust is to collect the various engine results first and only compare them against the output if they agree. *This is implemented now*. - [x] rust-analyzer/expect-test#20 ## Nice to haves - [x] Don't use raw strings with a hash unconditionally when blessing (rust-analyzer/expect-test#28). - [x] Shorter alias for `expect![[`? It's currently hard-coded into the `expect-test` crate (rust-analyzer/expect-test#26). - [ ] Remove newline escapes from existing tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently,
expect-testuses a raw string with at least one hash when blessing tests. This is a bit verbose for single-line output that doesn't contain any quotes or escaped characters.This PR parses each patch to find the simplest string literal that can represent it losslessly, and uses that instead. For now, multiline patches continue to use raw string literals (with the minimum number of hashes), although I think they could use regular ones.