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

Add test for where clause order #92332

Merged
merged 1 commit into from
Jan 2, 2022

Conversation

GuillaumeGomez
Copy link
Member

I didn't use @snapshot because of the   characters, it's much simpler doing it through rustdoc-gui testsuite.

r? @camelid

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 27, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2021
@GuillaumeGomez
Copy link
Member Author

Linked to #92296 and to #89098.

@camelid
Copy link
Member

camelid commented Dec 28, 2021

Please use @snapshot; this is not what rustdoc-gui is meant to be used for. I'd rather have htmldocck tests for things like this, and reserve rustdoc-gui for testing graphics (e.g., CSS styles).

I also imagine rustdoc-gui is somewhat slower since it spins up a browser, and rustdoc's tests are already quite slow.

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 28, 2021
@camelid camelid added the A-testsuite Area: The testsuite used to check the correctness of rustc label Dec 28, 2021
@GuillaumeGomez
Copy link
Member Author

Like I said, there are a lot of \0xa0 characters, making the test writing very long in the rustdoc testsuite and absolutely ugly to read. A test should be easy to read, that would go against that.

@GuillaumeGomez
Copy link
Member Author

Also: it would be very long because of all the links and tags (for syntax highlighting) in it. So I really don't think @snapshot is the right move here.

@camelid
Copy link
Member

camelid commented Dec 29, 2021

Can you try doing a snapshot test for the XPath //div[@id='impl-SomeTrait%3C(A%2C%20B%2C%20C%2C%20D%2C%20E)%3E-for-(A%2C%20B%2C%20C%2C%20D%2C%20E)']/h3/text()? I think it might get rid of the links.

@GuillaumeGomez
Copy link
Member Author

The goal here isn't to check for the DOM but for the clause order. So once again, I really don't think that using @snapshot is the right call here (that and the reasons listed above).

@camelid
Copy link
Member

camelid commented Dec 29, 2021

The goal here isn't to check for the DOM but for the clause order. So once again, I really don't think that using @snapshot is the right call here (that and the reasons listed above).

The goal is to check the DOM—GUI tests are for checking dynamic components of a page, not the generated HTML. I really don't think using GUI tests for this is good. Can you please at least post the output of the snapshot test? I'm not sure what you mean about there being lots of \0x0a characters.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 30, 2021

The goal is to check the DOM

No, the goal is to check the text to ensure that the where clause are generated in the correct order.

In any case, here what @snapshot looks like:

src/test/rustdoc/where-clause-order.rs:

#![crate_name = "foo"]

pub trait SomeTrait<Rhs = Self>
where Rhs: ?Sized
{}

// @has 'foo/trait.SomeTrait.html'
// @snapshot S1 - "//div[@id='impl-SomeTrait%3C(A%2C%20B%2C%20C%2C%20D%2C%20E)%3E-for-(A%2C%20B%2C%20C%\
2C%20D%2C%20E)']/h3"
impl<A, B, C, D, E> SomeTrait<(A, B, C, D, E)> for (A, B, C, D, E) where
    A: PartialOrd<A> + PartialEq<A>,
    B: PartialOrd<B> + PartialEq<B>,
    C: PartialOrd<C> + PartialEq<C>,
    D: PartialOrd<D> + PartialEq<D>,
    E: PartialOrd<E> + PartialEq<E> + ?Sized
{}

src/test/rustdoc/where-clause-order.S1.html

<h3 class="code-header in-band">impl&lt;A, B, C, D, E&gt; <a class="trait" href="trait.SomeTrait.html" \
title="trait foo::SomeTrait">SomeTrait</a>&lt;<a class="primitive" href="https://doc.rust-lang.org/nigh\
tly/std/primitive.tuple.html">(</a>A, B, C, D, E<a class="primitive" href="https://doc.rust-lang.org/ni\
ghtly/std/primitive.tuple.html">)</a>&gt; for <a class="primitive" href="https://doc.rust-lang.org/nigh\
tly/std/primitive.tuple.html">(</a>A, B, C, D, E<a class="primitive" href="https://doc.rust-lang.org/ni\
ghtly/std/primitive.tuple.html">)</a> <span class="where fmt-newline">where<br />&#160;&#160;&#160;&#16\
0;A: <a class="trait" href="https://doc.rust-lang.org/nightly/core/cmp/trait.PartialOrd.html" title="tr\
ait core::cmp::PartialOrd">PartialOrd</a>&lt;A&gt; + <a class="trait" href="https://doc.rust-lang.org/n\
ightly/core/cmp/trait.PartialEq.html" title="trait core::cmp::PartialEq">PartialEq</a>&lt;A&gt;,<br />&\
#160;&#160;&#160;&#160;B: <a class="trait" href="https://doc.rust-lang.org/nightly/core/cmp/trait.Parti\
alOrd.html" title="trait core::cmp::PartialOrd">PartialOrd</a>&lt;B&gt; + <a class="trait" href="https:\
//doc.rust-lang.org/nightly/core/cmp/trait.PartialEq.html" title="trait core::cmp::PartialEq">PartialEq\
</a>&lt;B&gt;,<br />&#160;&#160;&#160;&#160;C: <a class="trait" href="https://doc.rust-lang.org/nightly\
/core/cmp/trait.PartialOrd.html" title="trait core::cmp::PartialOrd">PartialOrd</a>&lt;C&gt; + <a class\
="trait" href="https://doc.rust-lang.org/nightly/core/cmp/trait.PartialEq.html" title="trait core::cmp:\
:PartialEq">PartialEq</a>&lt;C&gt;,<br />&#160;&#160;&#160;&#160;D: <a class="trait" href="https://doc.\
rust-lang.org/nightly/core/cmp/trait.PartialOrd.html" title="trait core::cmp::PartialOrd">PartialOrd</a\
>&lt;D&gt; + <a class="trait" href="https://doc.rust-lang.org/nightly/core/cmp/trait.PartialEq.html" ti\
tle="trait core::cmp::PartialEq">PartialEq</a>&lt;D&gt;,<br />&#160;&#160;&#160;&#160;E: <a class="trai\
t" href="https://doc.rust-lang.org/nightly/core/cmp/trait.PartialOrd.html" title="trait core::cmp::Part\
ialOrd">PartialOrd</a>&lt;E&gt; + <a class="trait" href="https://doc.rust-lang.org/nightly/core/cmp/tra\
it.PartialEq.html" title="trait core::cmp::PartialEq">PartialEq</a>&lt;E&gt; + ?<a class="trait" href="\
https://doc.rust-lang.org/nightly/core/marker/trait.Sized.html" title="trait core::marker::Sized">Sized\
</a>,&#160;</span></h3>

Just like I said: it's not helpful at all to ensure the where clauses are sorted as expected...

@camelid
Copy link
Member

camelid commented Jan 1, 2022

I still don't think that GUI tests are the right way to test this, but for some reason /text() isn't working with snapshot tests. I'm going to re-assign this to someone more knowledgeable with GUI tests since I have a bunch of PRs assigned to me.

r? @jsha

@rust-highfive rust-highfive assigned jsha and unassigned camelid Jan 1, 2022
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 1, 2022
@bors
Copy link
Contributor

bors commented Jan 1, 2022

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

@jsha
Copy link
Contributor

jsha commented Jan 1, 2022

Here's a test against the HTML (i.e. not a GUI test) that passes and is equally as clear as the GUI test. It doesn't use @snapshot. @GuillaumeGomez would you be willing to do this instead?

src/test/rustdoc/where-clause-order.rs

#![crate_name = "foo"]

pub trait SomeTrait<Rhs = Self>
where Rhs: ?Sized
{}

// @has 'foo/trait.SomeTrait.html'
// @has - "//div[@id='impl-SomeTrait%3C(A%2C%20B%2C%20C%2C%20D%2C%20E)%3E-for-(A%2C%20B%2C%20C%2C%20D%2C%20E)']/h3" "impl<A, B, C, D, E> SomeTrait<(A, B, C, D, E)> for (A, B, C, D, E) where A: PartialOrd<A> + PartialEq<A>, B: PartialOrd<B> + PartialEq<B>, C: PartialOrd<C> + PartialEq<C>, D: PartialOrd<D> + PartialEq<D>, E: PartialOrd<E> + PartialEq<E> + ?Sized, "
impl<A, B, C, D, E> SomeTrait<(A, B, C, D, E)> for (A, B, C, D, E) where
    A: PartialOrd<A> + PartialEq<A>,
    B: PartialOrd<B> + PartialEq<B>,
    C: PartialOrd<C> + PartialEq<C>,
    D: PartialOrd<D> + PartialEq<D>,
    E: PartialOrd<E> + PartialEq<E> + ?Sized
{}

@GuillaumeGomez
Copy link
Member Author

Yes it's fine. The reading isn't great but doesn't matter.

@GuillaumeGomez
Copy link
Member Author

Done!

@jsha
Copy link
Contributor

jsha commented Jan 1, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 1, 2022

📌 Commit ec0c838 has been approved by jsha

@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 1, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 2, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#84083 (Clarify the guarantees that ThreadId does and doesn't make.)
 - rust-lang#91593 (Remove unnecessary bounds for some Hash{Map,Set} methods)
 - rust-lang#92297 (Reduce compile time of rustbuild)
 - rust-lang#92332 (Add test for where clause order)
 - rust-lang#92438 (Enforce formatting for rustc_codegen_cranelift)
 - rust-lang#92463 (Remove pronunciation guide from Vec<T>)
 - rust-lang#92468 (Emit an error for `--cfg=)`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a015c86 into rust-lang:master Jan 2, 2022
@rustbot rustbot added this to the 1.59.0 milestone Jan 2, 2022
@GuillaumeGomez GuillaumeGomez deleted the where-clause-order branch January 2, 2022 17:05
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 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
Development

Successfully merging this pull request may close these issues.

6 participants