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

new tool rustdoc-gui-test #111348

Merged
merged 6 commits into from
May 27, 2023

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented May 8, 2023

Implements new tool rustdoc-gui-test that allows using compiletest headers for rustdoc-gui tests.

@rustbot
Copy link
Collaborator

rustbot commented May 8, 2023

Failed to set assignee to albertlarsan68: cannot assign: HTTP status server error (502 Bad Gateway) for url (https://api.github.com/repos/rust-lang/rust/issues/111348/assignees)

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot
Copy link
Collaborator

rustbot commented May 8, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 8, 2023
@rust-log-analyzer

This comment has been minimized.

@onur-ozkan onur-ozkan force-pushed the remove-hardcoded-rustdoc-flags branch 2 times, most recently from 84e2def to 380b7d7 Compare May 8, 2023 14:53
@onur-ozkan
Copy link
Member Author

How does this change looks like ? Is it overkill to use compiletest_rs for parsing the headers? I didn't want to implement it again to avoid code duplication in the repository.

cc @jyn514

@onur-ozkan onur-ozkan changed the title [wip] remove hardcoded RUSTDOCFLAGS flags in bootstrap [wip] implement compiletest headers to rustdoc-gui May 8, 2023
@onur-ozkan onur-ozkan force-pushed the remove-hardcoded-rustdoc-flags branch 2 times, most recently from b8431c2 to ccfb1e8 Compare May 8, 2023 15:07
@onur-ozkan onur-ozkan marked this pull request as ready for review May 8, 2023 15:07
@onur-ozkan onur-ozkan changed the title [wip] implement compiletest headers to rustdoc-gui implement compiletest headers to rustdoc-gui May 8, 2023
@jyn514
Copy link
Member

jyn514 commented May 8, 2023

How big is the function you're using? If it's not giant, I'd rather move just that function to build_helper so we don't have to pull in all of compiletest.

@onur-ozkan
Copy link
Member Author

How big is the function you're using? If it's not giant, I'd rather move just that function to build_helper so we don't have to pull in all of compiletest.

Moving is even more complex because it requires a lot of functions and types to move. Alternative way to adding compiletest_rs dependency is writing straight forward parsing function with simple config type for compile-args and run-flags. But that makes me feel uncomfortable since it requires maintaining multiple parsing implementations(with different algorithms) that does the same thing.

@Mark-Simulacrum
Copy link
Member

I think the right solution is to move rustdoc-gui to src/tools/compiletest so that all flags can be supported.

@onur-ozkan
Copy link
Member Author

onur-ozkan commented May 8, 2023

I think the right solution is to move rustdoc-gui to src/tools/compiletest so that all flags can be supported.

They are quite different things IMO. rustdoc-gui has tests that are defined as crates, and the way it performs tests is completely different(uses goml files with browser-ui-test npm package) than the rest. We can still support all the flags from compiletest_rs

--

If pulling compiletest from https://github.com/Manishearth/compiletest-rs is the problem, maybe we can use compiletest locally from src/tools/compiletest?

@Mark-Simulacrum
Copy link
Member

A separate crate like compiletest (e.g., src/tools/rustdoc-gui) would avoid everyone building Rust needing to compile the extra dependencies, if you don't want to build it into compiletest proper.

@onur-ozkan onur-ozkan marked this pull request as draft May 8, 2023 16:32
@onur-ozkan onur-ozkan changed the title implement compiletest headers to rustdoc-gui create new tool rustdoc-gui-tests May 8, 2023
@onur-ozkan onur-ozkan changed the title create new tool rustdoc-gui-tests create new tool rustdoc-gui-test May 8, 2023
@onur-ozkan onur-ozkan 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 May 8, 2023
@onur-ozkan onur-ozkan changed the title create new tool rustdoc-gui-test new tool rustdoc-gui-test May 9, 2023
@onur-ozkan
Copy link
Member Author

@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 9, 2023
@oli-obk
Copy link
Contributor

oli-obk commented May 26, 2023

@bors r=albertlarsan68,oli-obk

@bors
Copy link
Contributor

bors commented May 26, 2023

📌 Commit 0fb171795ce80fae65d381521d5bafcc0937affa has been approved by albertlarsan68,oli-obk

It is now in the queue for this repository.

@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. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 26, 2023
@onur-ozkan
Copy link
Member Author

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 26, 2023
@onur-ozkan onur-ozkan force-pushed the remove-hardcoded-rustdoc-flags branch from 0fb1717 to c7cec29 Compare May 26, 2023 14:00
@onur-ozkan
Copy link
Member Author

@bors r=albertlarsan68,oli-obk

@bors
Copy link
Contributor

bors commented May 26, 2023

📌 Commit c7cec29 has been approved by albertlarsan68,oli-obk

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 26, 2023
…-flags, r=albertlarsan68,oli-obk

new tool `rustdoc-gui-test`

Implements new tool `rustdoc-gui-test` that allows using compiletest headers for `rustdoc-gui` tests.
@bors
Copy link
Contributor

bors commented May 27, 2023

⌛ Testing commit c7cec29 with merge dbfc95f...

@bors
Copy link
Contributor

bors commented May 27, 2023

☀️ Test successful - checks-actions
Approved by: albertlarsan68,oli-obk
Pushing dbfc95f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2023
@bors bors merged commit dbfc95f into rust-lang:master May 27, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 27, 2023
@onur-ozkan onur-ozkan deleted the remove-hardcoded-rustdoc-flags branch May 27, 2023 07:08
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (dbfc95f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.0% [1.0%, 1.0%] 1
Regressions ❌
(secondary)
2.1% [2.1%, 2.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.0% [1.0%, 1.0%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 645.695s -> 643.935s (-0.27%)

@@ -8,13 +8,13 @@ set-window-size: (600, 800)
assert-property: ("html", {"scrollTop": "0"})

click: '//a[text() = "barbar"]'
assert-property: ("html", {"scrollTop": "125"})
assert-property: ("html", {"scrollTop": "149"})
Copy link
Member

Choose a reason for hiding this comment

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

Why did these values change? Unless you changed the default size of the page, they should not have been updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks! But why generating the links to def all the time then? Was there an issue to generate it only for one crate? It's a nightly-only option, so it should not be included all the time because it prevents us to test the almost 100% use-case, which is problematic imo.

@GuillaumeGomez
Copy link
Member

Would have been nice if the rustdoc team was contacted for the changes in the rustdoc GUI test. I'm very nervous about this change as I don't understand why it's needed.

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Jun 1, 2023

I don't understand why it's needed.

#111348 (comment)

@onur-ozkan
Copy link
Member Author

Would have been nice if the rustdoc team was contacted for the changes in the rustdoc GUI test.

You are right, we forgot doing that, sorry about that.

The main reason for having this tool was to prevent the need for manually inserting test flags in bootstrap side. If we were to directly use compiletest in bootstrap, it would have made the bootstrapping process longer.

@GuillaumeGomez
Copy link
Member

I'm fine with the tool (and nice job!), just would have preferred to be pinged for the GUI test change to at least be aware of it.

@onur-ozkan
Copy link
Member Author

just would have preferred to be pinged for the GUI test change to at least be aware of it.

absoluletely true, again sorry for missing that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants