Add test for line-number setting #90048
Conversation
GuillaumeGomez
added
the
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
| "margin": "0px", | ||
| "padding": "13px 8px", | ||
| "text-align": "right", | ||
| "border-top-left-radius": "5px", | ||
| "border-bottom-left-radius": "5px", | ||
| "font-size": "16px", | ||
| "font-family": '"Source Code Pro", monospace' |
There was a problem hiding this comment.
This will make the test fragile. Any time we change these CSS properties, the test will break. I recommend choosing a thing or two that is semantically meaningful for the line numbers. For instance, text-align should always be right; if we miss that, things will look bad.
Also, can we check that the contents are a number? Maybe a specific number, like 1 for the first line?
There was a problem hiding this comment.
Actually the <pre> contains all the lines at once separated by \n. But I can add a check. :)
There was a problem hiding this comment.
Also, the line numbers are a "hidden" feature (not enabled by default) and were not tested before this test (not great...). Considering it will be the only existing test for this feature, I think it makes sense to have one strict test to ensure it won't be forgotten and that any change that would impact it actually need to see why it broke.
There was a problem hiding this comment.
Checking unrelated properties (like border radius) doesn't make a test better, it just makes it more fragile. Fragility causes a test to fail when someone changes something, but didn't actually break anything.
I think of tests like an alerting system: If they alert all the time, maintainers and contributors (a) get warning blindness, (b) start to disrespect the tests, and (c) start to mechanically update them whenever they fail, rather than looking at what actually caused them to break.
You don't want your tests to signal "something changed;" you want them to signal "something broke."
There was a problem hiding this comment.
Something changed in this case means that something broke. It also seems very unlikely that any of these properties changes unless you're working on code blocks. And in this case, you definitely want to be aware of this feature.
There was a problem hiding this comment.
I removed the border-radius though since changing it wouldn't be a big issue...
There was a problem hiding this comment.
Not everything that changes means something broke related to the line numbers. For instance: right now, the line numbers inherit their (calculated) font-size of 16em from a rule in normalize.css:
pre {
font-size: 1em;
}
Imagine that changes to 2em. The will break a lot of our layouts, not just the line numbers. If that's something we're worried about (I don't think we are), we would want one test to fail: a test that checks that the contents of our <pre> tags are the right size. If instead a handful of tests about different features, like line numbers, failed, that would be very confusing and would result in the PR author thinking our tests are not meaningful. They might even give up their PR in frustration.
Another example: you're testing for the font face. That's set for all code and pre by this rule in rustdoc.css:
code, pre, a.test-arrow, .code-header {
font-family: "Source Code Pro",monospace;
}
If we consciously decide to switch fonts for our pre tags in the future, we know we're changing fonts - we don't need a line-numbers unit test to tell us "this specific pre tag, out of all our pre tags, had a changed font."
There was a problem hiding this comment.
Makes sense, I removed these checks.
GuillaumeGomez
force-pushed
the
line-number-setting
branch
from
a68b600 to
6dccdd8
Compare
|
Added the text check. |
GuillaumeGomez
force-pushed
the
line-number-setting
branch
2 times, most recently
from
72319ed to
fa5f08b
Compare
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
line-number-setting
branch
from
fa5f08b to
457f578
Compare
|
@bors r+ rollup |
|
📌 Commit 457f578 has been approved by |
bors
added
S-waiting-on-bors
… r=jsha Add test for line-number setting The first commit updates the version of the package to be able to have multi-line commands (which looks much nicer for this test). r? `@jsha`
… r=jsha Add test for line-number setting The first commit updates the version of the package to be able to have multi-line commands (which looks much nicer for this test). r? ``@jsha``
… r=jsha Add test for line-number setting The first commit updates the version of the package to be able to have multi-line commands (which looks much nicer for this test). r? ```@jsha```
Rollup of 14 pull requests Successful merges: - rust-lang#86984 (Reject octal zeros in IPv4 addresses) - rust-lang#87440 (Remove unnecessary condition in Barrier::wait()) - rust-lang#88644 (`AbstractConst` private fields) - rust-lang#89292 (Stabilize CString::from_vec_with_nul[_unchecked]) - rust-lang#90010 (Avoid overflow in `VecDeque::with_capacity_in()`.) - rust-lang#90029 (Add test for debug logging during incremental compilation) - rust-lang#90031 (config: add the option to enable LLVM tests) - rust-lang#90048 (Add test for line-number setting) - rust-lang#90071 (Remove hir::map::blocks and use FnKind instead) - rust-lang#90074 (2229 migrations small cleanup) - rust-lang#90077 (Make `From` impls of NonZero integer const.) - rust-lang#90097 (Add test for duplicated sidebar entries for reexported macro) - rust-lang#90098 (Add test to ensure that the missing_doc_code_examples is not triggered on foreign trait implementations) - rust-lang#90099 (Fix MIRI UB in `Vec::swap_remove`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The first commit updates the version of the package to be able to have multi-line commands (which looks much nicer for this test).
r? @jsha