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

"]]" inside an expect breaks updating #20

Closed
Tracked by #744
ecstatic-morse opened this issue Dec 23, 2021 · 2 comments · Fixed by #23
Closed
Tracked by #744

"]]" inside an expect breaks updating #20

ecstatic-morse opened this issue Dec 23, 2021 · 2 comments · Fixed by #23

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Dec 23, 2021 •
edited
Loading

The current code does a simple search for the character sequence ]] to determine the end of an expect![[ macro invocation. However, expect takes a string, and "]]" is perfectly valid as part of a string.
@matklad
Copy link
Member

matklad commented Dec 23, 2021

Yeah… we should’t be adding a real Rust parser here, but it makes sense to make heuristic more robust to the practical cases. Here, I think it makes sense to verify that symbols before ]] are quote/hash and whotespace

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Dec 23, 2021 •
edited
Loading

we should’t be adding a real Rust parser here

@matklad Interesting. I thought this would be the solution. Is this just to avoid a syn dependency? You could also go half way and write/steal a tokenizer for just string literals. I'll write the simple regex in the meantime. Maybe we can discuss on the PR.

This was referenced Dec 23, 2021
Closed
Merged
Merged
@bors bors bot closed this as completed in c60f9a0 Dec 25, 2021
bors added a commit to rust-lang/chalk that referenced this issue Feb 13, 2022
@bors
Auto merge of #744 - ecstatic-morse:snap-test, r=jackh726
de7fcd0 
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
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Parse string literals to find their length ecstatic-morse/expect-test
2 participants
@matklad @ecstatic-morse