Implement @snapshot check for htmldocck
#91209
Conversation
|
cc @jsha, I know you had reservations about this |
|
Reporting back from the Zulip thread: I'm okay proceeding with this, with the intention to be really aware of how brittle the tests are, and to make a different plan if they are too brittle. We are starting out with the intention to make sure snapshot tests don't encompass too large an HTML subtree. What do you think of enforcing that in htmldocck? I.e., error out if there are more than N nodes, or more than X bytes in a snapshot? Also, the error output when this fails should say what the command is to bless the results. |
Tidy seems like a better place to put this, we should be avoiding python where possible imo. |
|
Except that python has the information whereas we would need a weird check in tidy to enforce it. |
I thought about doing the byte-based check, though making it node-based sounds like a better idea. I do worry that that will make the test brittle in a different way—as soon as the threshold is reached, perhaps by a PR that changes the DOM structure, a bunch of tests will fail and can't be fixed easily. But perhaps it's worth it? What do you think? I agree with Guillaume that if we implement this limit, it should be in htmldocck for simplicity. At some point it'd be nice to have htmldocck all be in Rust, but I think htmldocck is the best place for now. |
|
Why not both? |
What both? |
|
Bytes and node depth. |
|
Why? I think doing both would add needless complexity for not much gain. I think the node-based limit is probably a better estimate, since you may want to have a section with a lot of prose but few elements. |
|
Well, if you have more than X node depth and/or X bytes of data, abort. Don't see what it changes... The node depth should be the most important limit though. |
This is a good point. Maybe we could have the equivalent of a |
Hmm, now this sounds like tidy should be involved, as Joshua suggested. There's already |
|
Actually, I think integrating with tidy would be difficult, since it's not immediately obvious what files are the output of snapshot tests. @jsha is it okay to skip the limit for now? If we run into issues with brittle tests, we can always add a limit later. I'd like this PR to just implement an "MVP" of snapshot tests, and I want to avoid it getting too big. |
|
Ok by me. Thanks for giving it a look.
|
This form of check allows performing snapshot tests (à la `src/test/ui`) on rustdoc HTML output, making it easier to create and update tests. See this Zulip thread [1] for more information about the motivation for this change. [1]: https://zulip-archive.rust-lang.org/stream/266220-rustdoc/topic/HTML.20snapshot.20tests.html#262651142
I'd been thinking about implementing snapshot testing for a while, but This test is what finally made me do it. It really benefits from using snapshot testing, so it's a good initial place to use `@snapshot`.
|
I fixed Joshua's review comment and added docs. |
|
This should be ready for a final review now :) |
| // 'Hello world!' | ||
| // @has - '//details[@class="rustdoc-toggle top-doc"]/div[@class="docblock"]/p[2]' \ | ||
| // 'Goodbye! Hello again!' | ||
| // @snapshot S2_top-doc - '//details[@class="rustdoc-toggle top-doc"]/div[@class="docblock"]' |
There was a problem hiding this comment.
Wouldn't it make more sense to keep the original order for the argument? So first, the file we're checking and then the snapshot file. It would be incoherent with the rest otherwise.
There was a problem hiding this comment.
I considered that, but then it looks a bit like S2_top-doc is a text pattern to match against. Plus, the name of the snapshot is important so you know what file to look at it for the output, so I put it at the front. I'm open to changing it, but I'm not sure if putting the snapshot name at the end would necessarily be clearer.
There was a problem hiding this comment.
I tihnk coherence is the most important thing. For me it's super strange that only @snapshot has this order for the args. If others agree with you, I won't oppose though.
There was a problem hiding this comment.
Joshua gave a thumbs up on my previous message, so I think let's keep it at the beginning for now if that's okay.
There was a problem hiding this comment.
Can always be changed later on in any case so it's fine.
|
@bors r+ |
|
📌 Commit fe88fcf has been approved by |
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#90538 (Document how recursion is handled for `ty::Ty`) - rust-lang#90851 (Add unchecked downcast methods) - rust-lang#91209 (Implement ``@snapshot`` check for htmldocck) - rust-lang#91385 (Suggest the `pat_param` specifier before `|` on 2021 edition ) - rust-lang#91478 (Remove incorrect newline from float cast suggestion) - rust-lang#91481 (Use let_else in some more places in rustc_lint) - rust-lang#91488 (Fix ICE when `yield`ing in function returning `impl Trait`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This form of check allows performing snapshot tests (à la
src/test/ui)on rustdoc HTML output, making it easier to create and update tests.
See this Zulip thread for more information about the motivation for
this change.
r? @GuillaumeGomez