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 back help-page.goml rustdoc GUI test #127082

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

GuillaumeGomez
Copy link
Member

Since #127010 was merged, let's see if we can revert #126445...

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 28, 2024
@GuillaumeGomez
Copy link
Member Author

@bors rollup=iffy

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

If only I could know what's going on in this server...

@GuillaumeGomez
Copy link
Member Author

Like this it works. Makes me sad but better that than not having this test.

@notriddle
Copy link
Contributor

Try this change?

diff --git a/tests/rustdoc-gui/utils.goml b/tests/rustdoc-gui/utils.goml
index 844dc98a537..7bf3fe05772 100644
--- a/tests/rustdoc-gui/utils.goml
+++ b/tests/rustdoc-gui/utils.goml
@@ -13,6 +13,6 @@ define-function: (
         // Close the popover.
         click: "#settings-menu"
         // Ensure that the local storage was correctly updated.
-        assert-local-storage: {"rustdoc-theme": |theme|}
+        wait-for-local-storage: {"rustdoc-theme": |theme|}
     },
 )

@GuillaumeGomez
Copy link
Member Author

I already tried it in #126436.

@notriddle
Copy link
Contributor

Okay, try this one:

diff --git a/src/librustdoc/html/static/js/settings.js b/src/librustdoc/html/static/js/settings.js
index 2b42fbebb80..176f6f86b20 100644
--- a/src/librustdoc/html/static/js/settings.js
+++ b/src/librustdoc/html/static/js/settings.js
@@ -219,6 +219,9 @@
             document.getElementById(MAIN_ID).appendChild(el);
         } else {
             el.setAttribute("tabindex", "-1");
+            // Don't make the popover visible until after the event handlers are set up.
+            // Otherwise, we get flakey integration tests.
+            el.style.display = "none";
             getSettingsButton().appendChild(el);
         }
         return el;
diff --git a/tests/rustdoc-gui/utils.goml b/tests/rustdoc-gui/utils.goml
index 844dc98a537..ebf0c65bd14 100644
--- a/tests/rustdoc-gui/utils.goml
+++ b/tests/rustdoc-gui/utils.goml
@@ -7,12 +7,12 @@ define-function: (
         // Open the settings menu.
         click: "#settings-menu"
         // Wait for the popover to appear...
-        wait-for: "#settings"
+        wait-for-css: ("#settings", {"display": "block"})
         // Change the setting.
         click: "#theme-"+ |theme|
         // Close the popover.
         click: "#settings-menu"
         // Ensure that the local storage was correctly updated.
-        assert-local-storage: {"rustdoc-theme": |theme|}
+        wait-for-local-storage: {"rustdoc-theme": |theme|}
     },
 )

The interesting part, the change in settings.js, is based on this hypothesis:

  1. buildSettingsPage is running synchronously when the script is first run
  2. The test harness is instantly clicking the radio button after it becomes visible
  3. Then the event handlers are registered in the setTimeout tick below, failing to respond to the form inputs in step 2

To avoid this, I ensure the settings page isn't visible until after the event handlers are registered.

@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@GuillaumeGomez
Copy link
Member Author

Let's see if it works.

@GuillaumeGomez
Copy link
Member Author

Ah, github actions seems to be having issues. Suspense instensifies

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jun 29, 2024

Same failure. Dark magic, really. I have no clue why it just doesn't work in the CI.

@notriddle
Copy link
Contributor

K

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 29, 2024

📌 Commit c8bbeef has been approved by notriddle

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. labels Jun 29, 2024
@bors
Copy link
Contributor

bors commented Jul 1, 2024

⌛ Testing commit c8bbeef with merge ad12a2a...

@bors
Copy link
Contributor

bors commented Jul 1, 2024

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing ad12a2a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 1, 2024
@bors bors merged commit ad12a2a into rust-lang:master Jul 1, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 1, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ad12a2a): 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)

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

Cycles

Results (secondary 4.4%)

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)
- - 0
Regressions ❌
(secondary)
4.4% [3.5%, 6.4%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 696.913s -> 697.036s (0.02%)
Artifact size: 324.66 MiB -> 324.65 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-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.

7 participants