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 new rustdoc-GUI to ensure correct font is used on module items #85669

Conversation

GuillaumeGomez
Copy link
Member

Fixes #85632.

r? @jsha

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

jsha commented May 26, 2021

This test seems fragile: it's testing a specific DOM layout. For instance it will be immediately broken by #85651. As I said on #85651, we should give a specific class to things, e.g. <a class="module-item-name">...</a>, and then write a CSS selector against that class. Then we can also have this test target that class, although at that point it winds up being kind of a test for "do we have this CSS rule and does an element with this class exist."

@GuillaumeGomez
Copy link
Member Author

Contrary to the CSS rules, I think the test on the opposite should be very layout specific so then when you make a modification, it'll break and you'll be forced to update it and check if it's still working as expected.

@jsha
Copy link
Contributor

jsha commented May 29, 2021

when you make a modification, it'll break and you'll be forced to update it and check if it's still working as expected.

When tests break any time you make a modification, and have a simple update path ("just update the expected output to the new output"), they don't actually serve to prevent breakages. People see the test failure, say "oh yeah that's expected because I changed the DOM," and update the test. Brittle tests also undermine people's faith that test failures are meaningful and important, and they create a barrier to new people entering a project - since it may not be as clear to a new person why the tests are failing or how to fix them.

It's much better to have flexible tests that test a semantic property rather than a structural one. For instance, in this case the real properties we are trying to verify are things like: "the string WhoLetTheDogOut shows up in the DOM after the Enums heading with a font of Fira Sans"; "the string 'Just a normal enum' show up in the DOM after the Enums heading with a Font of Source Serif 4".

We have a very good example in #85651 (comment). @dns2utf8 is making the same sort of change I was making when I originally broke the font selection for item names in search results: a <table> layout becomes some other sort of element (an <item-table>). What sort of test failure would be better to receive in such a situation:

  1. The test says "I expected there to be a #enums + table td > a with font 'Fira Sans'"
  2. The test says "I expected there to be text WhoLetTheDogOut with font 'Fira Sans'"

The former fails, even if you correctly update the CSS. The latter succeeds if and only if you correctly update the CSS. And when it fails it tells you something more meaningful about the intent of the test - that item names should be rendered in our sans serif font.

@GuillaumeGomez
Copy link
Member Author

When tests break any time you make a modification, and have a simple update path ("just update the expected output to the new output"), they don't actually serve to prevent breakages. People see the test failure, say "oh yeah that's expected because I changed the DOM," and update the test. Brittle tests also undermine people's faith that test failures are meaningful and important, and they create a barrier to new people entering a project - since it may not be as clear to a new person why the tests are failing or how to fix them.

This is the same for more relaxed tests though.

I guess I'm afraid that a change would break something but that the selector would still point to something "valid" from the test point of view. Well, you're more experienced than me so let's go with your suggestion!

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2021
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-testsuite Area: The testsuite used to check the correctness of rustc and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2021
@JohnCSimon
Copy link
Member

JohnCSimon commented Aug 9, 2021

Triage:
@GuillaumeGomez Any status? Is this ready for review?

@dns2utf8
Copy link
Contributor

dns2utf8 commented Aug 9, 2021

Now that the mobile first <item-table> is merged we can update the tests to check for the fonts there.

The main difference is that because of the dynamic layout we no longer have a common grouping element (formerly <tr>) which is not important for these tests here.

I quickly threw this together, so it is not tested with the latest commit:

goto: file://|DOC_PATH|/test_docs/index.html
// This test ensures that the correct font is applied on the modules listed items.

// modules
assert-css: ("#modules + item-table .item-left a", {"font-family": '"Fira Sans", Arial, sans-serif'})
assert-css: ("#modules + item-table .item-right.docblock-short", {"font-family": '"Source Serif 4", serif'})
// structs
assert-css: ("#structs + item-table .item-left a", {"font-family": '"Fira Sans", Arial, sans-serif'})
assert-css: ("#structs + item-table .item-right.docblock-short", {"font-family": '"Source Serif 4", serif'})
// enums
assert-css: ("#enums + item-table .item-left a", {"font-family": '"Fira Sans", Arial, sans-serif'})
assert-css: ("#enums + item-table .item-right.docblock-short", {"font-family": '"Source Serif 4", serif'})
// traits
assert-css: ("#traits + item-table .item-left a", {"font-family": '"Fira Sans", Arial, sans-serif'})
assert-css: ("#traits + item-table .item-right.docblock-short", {"font-family": '"Source Serif 4", serif'})
// functions
assert-css: ("#functions + item-table .item-left a", {"font-family": '"Fira Sans", Arial, sans-serif'})
assert-css: ("#functions + item-table .item-right.docblock-short", {"font-family": '"Source Serif 4", serif'})
// keywords
assert-css: ("#keywords + item-table .item-left a", {"font-family": '"Fira Sans", Arial, sans-serif'})
assert-css: ("#keywords + item-table .item-right.docblock-short", {"font-family": '"Source Serif 4", serif'})

@jyn514
Copy link
Member

jyn514 commented Aug 16, 2021

@dns2utf8 are you interested in making a PR with that test?

@jyn514 jyn514 closed this Aug 16, 2021
@jyn514 jyn514 reopened this Aug 16, 2021
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Updating the tests now. ;)

@GuillaumeGomez GuillaumeGomez force-pushed the new-rustdoc-gui-test-module-font branch from 790a285 to e701d8a Compare August 16, 2021 15:20
@GuillaumeGomez
Copy link
Member Author

Updated.

@dns2utf8
Copy link
Contributor

Sure, I will get on it unless @GuillaumeGomez has implemented it already

@dns2utf8 dns2utf8 mentioned this pull request Aug 16, 2021
dns2utf8 added a commit to dns2utf8/rust that referenced this pull request Aug 16, 2021
@dns2utf8
Copy link
Contributor

Very sorry for the confusion, I did not see the comments from @GuillaumeGomez for some reason 😅

@GuillaumeGomez
Copy link
Member Author

It's fine, we wrote almost exactly the same code (in #88089). Let's merge yours. ;)

@GuillaumeGomez GuillaumeGomez deleted the new-rustdoc-gui-test-module-font branch August 16, 2021 20:47
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Aug 16, 2021
…aumeGomez

Rustdoc font test

 Add a font test based on rust-lang#85669 fixes rust-lang#85632.

r? `@jsha` `@GuillaumeGomez`
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-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

Add GUI test for font on module table
7 participants