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

some low hanging clippy::perf fixes #88516

Merged
merged 1 commit into from Sep 2, 2021

Conversation

matthiaskrgr
Copy link
Member

No description provided.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

CI failure looks spurious, cc @GuillaumeGomez

@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2021

📌 Commit b257fc1e7b54a70156b8b02bdc663a6db2bd82cf has been approved by jyn514

@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 Aug 31, 2021
@GuillaumeGomez
Copy link
Member

Failure looks spurious, weird...

@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

@GuillaumeGomez right - my point is that test is flaky and we should find a way to make it less flaky. 30 seconds is already a long timeout so we probably don't want to increase it - is there a way to make whatever it's doing faster?

@GuillaumeGomez
Copy link
Member

There is no explicit timeout here, we just told the test to wait for a selector and that selector didn't appear in the (default) 30 seconds timeout. I think the problem is elsewhere.

@jyn514
Copy link
Member

jyn514 commented Aug 31, 2021

Hmm, ok. Maybe it's not spurious then. @matthiaskrgr can you run this locally and see whether it passes?

@bors r-

@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 Aug 31, 2021
@matthiaskrgr
Copy link
Member Author

./x.py test rustdoc succeeds locally. I rebased, let's see if that changes anything.

@GuillaumeGomez
Copy link
Member

./x.py test rustdoc succeeds locally. I rebased, let's see if that changes anything.

Is it running rustdoc-gui? If not we might want to fix that (I never ran it like this). Otherwise: x.py test src/test/rustdoc-gui --stage 1.

@matthiaskrgr
Copy link
Member Author

Ah no, apparently x.py test rustdoc does not run the rustdoc-gui tests. (got an error because of some missing npm package).
But I managed to reproduce the test failure at least! :)

@GuillaumeGomez
Copy link
Member

Time to fix it then! :p

@matthiaskrgr
Copy link
Member Author

So, I went through all the changes one by one now and applied them and I got a lot of spurious test failures:

code-sidebar-toggle... FAILED
[ERROR] (line 3) Error: No node found for selector: #sidebar-toggle: for command `click: "#sidebar-toggle"` 

sidebar... FAILED
[ERROR] (line 44) expected 1 elements, found 0: for command `assert-count: (".sidebar .location", 1)`

but after running ./x.py test src/test/rustdoc-gui --stage 1 a couple of times I always got to a test run which succeeded at some point, so to me it looks like the tests are just flaky :/

@GuillaumeGomez
Copy link
Member

This is highly surprising and a bit worrying. Well, if it isn't due to changes in this PR, let's move forward. I'll keep a close eye on such flakiness in the future.

@bors: r=jyn514,GuillaumeGomez

@bors
Copy link
Contributor

bors commented Sep 1, 2021

📌 Commit 7f2df9a has been approved by jyn514,GuillaumeGomez

@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 Sep 1, 2021
@GuillaumeGomez
Copy link
Member

Since it's changing performance:

@bors: rollup=iffy

@bors
Copy link
Contributor

bors commented Sep 2, 2021

⌛ Testing commit 7f2df9a with merge 579416e0c16c42239c4b83c759b8f9fcdd61611c...

@bors
Copy link
Contributor

bors commented Sep 2, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 2, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@jyn514
Copy link
Member

jyn514 commented Sep 2, 2021

Apple job failed without logs.

@bors retry

@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 Sep 2, 2021
@bors
Copy link
Contributor

bors commented Sep 2, 2021

⌛ Testing commit 7f2df9a with merge 38a9b8139bab93588db3cbe17f0bb309458ae965...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-distcheck failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Sep 2, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 2, 2021
@GuillaumeGomez
Copy link
Member

@bors: retry

@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 Sep 2, 2021
@bors
Copy link
Contributor

bors commented Sep 2, 2021

⌛ Testing commit 7f2df9a with merge 6492931...

@bors
Copy link
Contributor

bors commented Sep 2, 2021

☀️ Test successful - checks-actions
Approved by: jyn514,GuillaumeGomez
Pushing 6492931 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 2, 2021
@bors bors merged commit 6492931 into rust-lang:master Sep 2, 2021
@cuviper cuviper added this to the 1.56.0 milestone Nov 16, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants