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

htmldocck: Add support for /text() in @snapshot #92914

Merged
merged 1 commit into from
Jan 18, 2022
Merged

htmldocck: Add support for /text() in @snapshot #92914

merged 1 commit into from
Jan 18, 2022

Conversation

camelid
Copy link
Member

@camelid camelid commented Jan 15, 2022 •
edited
Loading

This allows just testing the text, in cases where the HTML tags don't
matter.

See #92908 (comment) for an example of when this would be useful.

r? @GuillaumeGomez

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 15, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2022
@camelid camelid added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jan 15, 2022
@camelid
Copy link
Member Author

camelid commented Jan 15, 2022

cc @jsha: I think this should mitigate your concern about @snapshot leading to brittle tests?

camelid
camelid commented Jan 15, 2022
View reviewed changes
src/etc/htmldocck.py
Comment on lines +500 to +503
if xpath.endswith('/text()'):
xpath = xpath[:-7]
normalize_to_text = True
Copy link
Member Author

Choose a reason for hiding this comment

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

This duplicates code elsewhere in htmldocck.

@camelid camelid mentioned this pull request Jan 15, 2022
Merged
@jsha
Copy link
Contributor

jsha commented Jan 15, 2022

How does this differ from our current XPath checks, where the stuff between the quotes is a text match? E.g.:

// @has cap_lints/index.html //* 'Crate cap_lints'
// @has cap_lints/struct.Foo.html //* 'Foo'

Also, is /text() valid XPath? Or does this move us further from a standard XPath deployment?

@camelid
Copy link
Member Author

camelid commented Jan 15, 2022 •
edited
Loading

How does this differ from our current XPath checks, where the stuff between the quotes is a text match? E.g.:

It makes the checks more readable and enables diffing and blessing (rather than just saying "XPath not found").

Also, is /text() valid XPath? Or does this move us further from a standard XPath deployment?

Yes, /text() is valid XPath. Also, /text() is already supported by @has, in a similarly hacky way.

@camelid
Copy link
Member Author

camelid commented Jan 15, 2022

An example of how the old way is not very readable:

// @has macro_generated_macro/macro.linebreak.html //pre 'macro_rules! linebreak {'
// @has - //pre '    ('
// @has - //pre '        <= 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25'
// @has - //pre '        26 27 28 =>'
// @has - //pre '    ) => { ... };'
// @has - //pre '};'

from an old version of #92908.

@jsha
Copy link
Contributor

jsha commented Jan 15, 2022

Looks good to me, then! I'll wait for @GuillaumeGomez to review, since they know these tests better than I.

@GuillaumeGomez
Copy link
Member

Yes, that seems good to me.

On a somewhat similar topic: it'd be nice that we merged #89676 at some point to really support XPath completely... Currently it's very hacky and pretty bad.

@camelid
htmldocck: Add support for /text() in @snapshot
9c6d8ef 
This allows just testing the text, in cases where the HTML tags don't
matter.
@GuillaumeGomez
Copy link
Member

I'm fine with this dd. Could you maybe a test making use of it please?

@dtolnay
Copy link
Member

dtolnay commented Jan 17, 2022

Is it good enough that I'll be adding tests making use of it in #92908?

@GuillaumeGomez
Copy link
Member

That sounds good to me!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jan 17, 2022

📌 Commit 9c6d8ef has been approved by GuillaumeGomez

@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 Jan 17, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 18, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2022
@bors
Auto merge of rust-lang#93021 - matthiaskrgr:rollup-o7z8zoe, r=matthi…
7531d2f 
…askrgr

Rollup of 14 pull requests

Successful merges:

 - rust-lang#92629 (Pick themes on settings page, not every page)
 - rust-lang#92640 (Fix ICEs related to `Deref<Target=[T; N]>` on newtypes)
 - rust-lang#92701 (Add some more attribute validation)
 - rust-lang#92803 (Hide mobile sidebar on some clicks)
 - rust-lang#92830 (Rustdoc style cleanups)
 - rust-lang#92866 ("Does exists" typos fix)
 - rust-lang#92870 (add `rustc_diagnostic_item` attribute to `AtomicBool` type)
 - rust-lang#92914 (htmldocck: Add support for `/text()` in ``@snapshot`)`
 - rust-lang#92923 (Abstract the pretty printer's ringbuffer to be infinitely sized)
 - rust-lang#92946 (Exclude llvm-libunwind from the self-contained set on s390x-musl targets)
 - rust-lang#92947 (rustdoc: Use `intersperse` in a `visit_path` function)
 - rust-lang#92997 (Add `~const` bound test for negative impls)
 - rust-lang#93004 (update codegen test for LLVM 14)
 - rust-lang#93016 (Stabilize vec_spare_capacity)

Failed merges:

 - rust-lang#92924 (Delete pretty printer tracing)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit be3d25b into rust-lang:master Jan 18, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 18, 2022
@camelid camelid deleted the snapshot-text branch January 18, 2022 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
7 participants
@camelid @jsha @GuillaumeGomez @dtolnay @bors @rust-highfive @rustbot