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

Gate shell_completions tests on availability #14541

Closed
wants to merge 1 commit into from

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Sep 13, 2024

This changes the shell_completions tests to only run if the corresponding external program is installed. Generally, cargo's tests shouldn't require any external commands to be installed.

This also uses the ignore attribute for disabling on macos. It is quite confusing when running on macos to see a successful test when it isn't running. This ensures that the test output indicates that it is ignored and the reason.

cc @shannmu

@rustbot
Copy link
Collaborator

rustbot commented Sep 13, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2024
@ehuss
Copy link
Contributor Author

ehuss commented Sep 13, 2024

Hm, so this approach isn't going to work. It's too late for me to continue looking at this, will maybe look at it tomorrow or the next few days.

@weihanglo
Copy link
Member

error: failed to install component: 'llvm-tools-preview-x86_64-unknown-linux-gnu', detected conflict: 'lib/rustlib/x86_64-unknown-linux-gnu/bin/llc'

@bors try

@bors
Copy link
Contributor

bors commented Sep 13, 2024

⌛ Trying commit fdf4f26 with merge fe6e210...

bors added a commit that referenced this pull request Sep 13, 2024
Gate shell_completions tests on availability

This changes the shell_completions tests to only run if the corresponding external program is installed. Generally, cargo's tests shouldn't require any external commands to be installed.

This also uses the `ignore` attribute for disabling on macos. It is quite confusing when running on macos to see a successful test when it isn't running. This ensures that the test output indicates that it is ignored and the reason.

cc `@shannmu`
@bors
Copy link
Contributor

bors commented Sep 13, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2024
@epage
Copy link
Contributor

epage commented Sep 13, 2024

I recommend we be free with reverts when a code change is blocking. We can always fix this in another pass.

@weihanglo
Copy link
Member

rust-lang/rust#130292 is merged. This shouldn't be blocked anymore.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 14, 2024

📌 Commit fdf4f26 has been approved by weihanglo

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: The marked PR is awaiting some action (such as code changes) from the PR author. labels Sep 14, 2024
bors added a commit that referenced this pull request Sep 14, 2024
Gate shell_completions tests on availability

This changes the shell_completions tests to only run if the corresponding external program is installed. Generally, cargo's tests shouldn't require any external commands to be installed.

This also uses the `ignore` attribute for disabling on macos. It is quite confusing when running on macos to see a successful test when it isn't running. This ensures that the test output indicates that it is ignored and the reason.

cc `@shannmu`
@bors
Copy link
Contributor

bors commented Sep 14, 2024

⌛ Testing commit fdf4f26 with merge 543b910...

@ehuss
Copy link
Contributor Author

ehuss commented Sep 14, 2024

@bors r-

This isn't going to work, since not all jobs have the shells installed.

I'm looking at a different approach where the jobs will be identified as being "extended" to test these extra things that might not be available.

@bors bors added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 14, 2024
@ehuss
Copy link
Contributor Author

ehuss commented Sep 14, 2024

And BTW, this will block updating in rust-lang/rust, since those won't have the shells installed either. I'm poking around now, but I'm a little confused why the fish test seems to be randomly failing.

@shannmu
Copy link
Contributor

shannmu commented Sep 14, 2024

The Fish shell test failures might be due to the test depending on the completest crate, which tests the completion results based on an interactive shell. We cannot be certain when the completion results are fully generated, so there is a built-in timeout parameter. If the completion results are not generated within the timeout period, it may cause the test to fail.

@ehuss
Copy link
Contributor Author

ehuss commented Sep 14, 2024

Hmm. That's unfortunate.

Let's just disable the tests for now. I have opened #14546 to do that, and #14545 lists the problems that I noticed while looking at this.

I'm probably not going to look at this further for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants