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

Introduce expect snapshot testing library into rustc #75773

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

matklad
Copy link
Member

@matklad matklad commented Aug 21, 2020

Snapshot testing is a technique for writing maintainable unit tests.
Unlike usual assert_eq! tests, snapshot tests allow
to automatically upgrade expected values on test failure.
In a sense, snapshot tests are inline-version of our beloved
UI-tests.

Example:

expect

A particular library we use, expect_test provides an expect!
macro, which creates a sort of self-updating string literal (by using
file! macro). Self-update is triggered by setting UPDATE_EXPECT
environmental variable (this info is printed during the test failure).
This library was extracted from rust-analyzer, where we use it for
most of our tests.

There are some other, more popular snapshot testing libraries:

The main differences of expect are:

  • first-class snapshot objects (so, tests can be written as functions,
    rather than as macros)
  • focus on inline-snapshots (but file snapshots are also supported)
  • restricted feature set (only assert_eq and assert_debug_eq)
  • no extra runtime (ie, no cargo insta)

See rust-lang/rust-analyzer#5101 for a
an extended comparison.

It is unclear if this testing style will stick with rustc in the long
run. At the moment, rustc is mainly tested via integrated UI tests.
But in the library-ified world, unit-tests will become somewhat more
important (that's why use use rustc_lexer library-ified library as
an example in this PR). Given that the cost of removal shouldn't be
too high, it probably makes sense to just see if this flies!

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2020
@matklad
Copy link
Member Author

matklad commented Aug 21, 2020

r? @Mark-Simulacrum

That's the thing i've mentioned in zullip

@mati865

This comment has been minimized.

@Mark-Simulacrum

This comment has been minimized.

@matklad

This comment has been minimized.

/** outer doc block */
/*! inner doc block */
",
expect![[r#"
Copy link
Member

Choose a reason for hiding this comment

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

Hm I wonder if this should be ... not a string? It seems rather unfortunate to lose all the syntax highlighting and autocompletion and such. Maybe expect_test could use stringify!?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've thought about that, but decided against. The problem with stringify! is that it erases whitesapce. While you still can use that for whitespace-insensitive comparison, producing a readable diff becomes hard.

It is possible to get whitespace back by reading-out the source of the file in case that you are going to produce an error anyway (which I actually didn't consider before).

Hm, yeah, I guess I still lean towards using just strings -- this is much simpler implementation wise, and I am not convinced that highlighting&completion are that much important -- the snippet in expect is inserted automatically, you rarely have to edit it by hand.

I'll be more worried about syntax highlighting in the input, which is genuinely valid rust-code. In rust-analyzer, we have a hack for that, but we need some form of stable custom attribute to generalize it (I guess we can use a no-op proc-macro, but that feels like an overkill for a syntax highlighting).

Copy link
Member

Choose a reason for hiding this comment

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

Ah right I missed that even the first time around it gets filled in automatically, that does make it much nicer.

Copy link
Member

Choose a reason for hiding this comment

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

I know vim at least colorizes/treats Markdown code blocks as "real rust code" (in Rust, anyway) -- I guess we could likely just rely on that, and either use a doc comment directly (with the proc macro parsing the markdown to retrieve the code block) or as you say a special attribute of some kind.

@Mark-Simulacrum
Copy link
Member

I'm going to just r+ this, but left a comment that I'd be happy to discuss separately (it's a fairly significant change to the API an upstream library, though one maintained by you I guess :)

@bors r+

@bors
Copy link
Contributor

bors commented Aug 21, 2020

📌 Commit 83c6b66c832b30535ec897fbcd009e965fecb197 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2020
@matklad
Copy link
Member Author

matklad commented Aug 21, 2020

Another important bit of info:

One problem with unit-test is that it might be awkward to setup "global state" to exercise a particular slice of compiler's functionality. The way we handle this in rust-analyzer is that we have a single "fixture" format, which allows us to describe source of a bunch of crates in a single string literal. We than have one function which sets up the context from this fixture, and all specific tests share this machinery.

Here's what fixture the fixture looks like for name resolution tests, for example (note syntax highlighting :-) ):

image

@Mark-Simulacrum
Copy link
Member

Yeah that does indeed look reasonable. I suspect in rustc we'd generally have such tests as UI tests with some kind of compiler flag to dump the resolution information (or just "compile pass").

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#75705 (Move to intra-doc links for /library/core/src/intrinsics.rs)
 - rust-lang#75711 (Split `astconv.rs` into its own submodule)
 - rust-lang#75718 (Don't count variants/fields/consts/associated types in doc-coverage doc examples)
 - rust-lang#75725 (Use intra-doc-links in `alloc`)
 - rust-lang#75745 (Remove duplication in `fold_item`)
 - rust-lang#75753 (Another motivation for CFG: return-oriented programming)
 - rust-lang#75769 (Minor, remove double nesting of a test module)
 - rust-lang#75771 (Extend normalization in const-eval-query-stack test)
 - rust-lang#75781 (More inline asm register name fixups for LLVM)
 - rust-lang#75782 (Convert core/src/str/pattern.rs to Intra-doc links)
 - rust-lang#75787 (Use intra-doc-links in `core::ops::*`)
 - rust-lang#75788 (MIR call terminator represents diverging calls too)

Failed merges:

 - rust-lang#75773 (Introduce expect snapshot testing library into rustc)

r? @ghost
@bors
Copy link
Contributor

bors commented Aug 22, 2020

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout snapshot-tests (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self snapshot-tests --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging src/librustc_lexer/src/tests.rs
CONFLICT (content): Merge conflict in src/librustc_lexer/src/tests.rs
Automatic merge failed; fix conflicts and then commit the result.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 22, 2020
@bors
Copy link
Contributor

bors commented Aug 22, 2020

☔ The latest upstream changes (presumably #75797) made this pull request unmergeable. Please resolve the merge conflicts.

Snapshot testing is a technique for writing maintainable unit tests.
Unlike usual `assert_eq!` tests, snapshot tests allow
to *automatically* upgrade expected values on test failure.
In a sense, snapshot tests are inline-version of our beloved
UI-tests.

Example:

![expect](https://user-images.githubusercontent.com/1711539/90888810-3bcc8180-e3b7-11ea-9626-d06e89e1a0bb.gif)

A particular library we use, `expect_test` provides an `expect!`
macro, which creates a sort of self-updating string literal (by using
`file!` macro). Self-update is triggered by setting `UPDATE_EXPECT`
environmental variable (this info is printed during the test failure).
This library was extracted from rust-analyzer, where we use it for
most of our tests.

There are some other, more popular snapshot testing libraries:

* https://github.com/mitsuhiko/insta
* https://github.com/aaronabramov/k9

The main differences of `expect` are:

* first-class snapshot objects (so, tests can be written as functions,
  rather than as macros)
* focus on inline-snapshots (but file snapshots are also supported)
* restricted feature set (only `assert_eq` and `assert_debug_eq`)
* no extra runtime (ie, no `cargo insta`)

See rust-lang/rust-analyzer#5101 for a
an extended comparison.

It is unclear if this testing style will stick with rustc in the long
run. At the moment, rustc is mainly tested via integrated UI tests.
But in the library-ified world, unit-tests will become somewhat more
important (that's why use use `rustc_lexer` library-ified library as
an example in this PR). Given that the cost of removal shouldn't be
too high, it probably makes sense to just see if this flies!
@matklad
Copy link
Member Author

matklad commented Aug 24, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 24, 2020

📌 Commit f7be59c has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 24, 2020
@bors
Copy link
Contributor

bors commented Aug 25, 2020

⌛ Testing commit f7be59c with merge c35007d...

@bors
Copy link
Contributor

bors commented Aug 25, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing c35007d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 25, 2020
@bors bors merged commit c35007d into rust-lang:master Aug 25, 2020
@matklad matklad deleted the snapshot-tests branch August 27, 2020 11:02
@matklad matklad restored the snapshot-tests branch August 27, 2020 11:10
matklad added a commit to matklad/rust that referenced this pull request Aug 29, 2020
@jyn514 jyn514 added the A-testsuite Area: The testsuite used to check the correctness of rustc label Sep 12, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants