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

shell_completions test issues #14545

Closed
Tracked by #14520
ehuss opened this issue Sep 14, 2024 · 3 comments · Fixed by #14590
Closed
Tracked by #14520

shell_completions test issues #14545

ehuss opened this issue Sep 14, 2024 · 3 comments · Fixed by #14590
Labels
A-completions Area: shell completions A-testing-cargo-itself Area: cargo's tests C-bug Category: bug S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@ehuss
Copy link
Contributor

ehuss commented Sep 14, 2024

Problem

The new shell_completions tests have a few issues that need to be resolved before they can be enabled.

  1. The tests randomly fail. I have seen fish and zsh randomly fail. Gate shell_completions tests on availability #14541 (comment) indicates that it might be an interactive timeout.
  2. They require external tools to be installed to run. Tests generally should not require that.
    • Looking for the binaries is not sufficient, since for example the bash test also requires the bash-completions package to be installed.
  3. The elvish test has a weird failure for me:
++++ actual:   In-memory
        1 + Deprecation: the legacy temporary assignment syntax is deprecated; use "tmp" instead
        2 +   /tmp/cargo/target/tmp/cit/t0/home/elvish/rc.elv:3:7: eval (E:CARGO_COMPLETE=elvish cargo | slurp)
  1. Tests should be using the ignore attribute so that it reports in the output when it isn't running. Currently they exit with return; on macos, but that should use the ignore attribute instead.
  2. Should figure out why the tests are ignored on macos, since I would expect that to work with homebrew. I'm unaware why these are currently ignored.

Steps

  1. cargo test --test testsuite -- shell_completions

Possible Solution(s)

One idea I had for limiting where these tests run is to add a CI_EXTENDED environment variable. The tests would only be required to run if that environment variable is set, and then we can set that in the appropriate jobs (like test). Then the cargo_test macro could have something like requires_extended_fish or something like that. That would also be useful for the currently hard-coded hg and lldb.

However, that doesn't help with some of the more complex issues like the bash-completions problem.

We could extend the cargo_test attribute to have ignore_macos to fix the macos ignore problem.

Notes

No response

Version

Currently on 643a025 of master.

@ehuss ehuss added C-bug Category: bug S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. A-completions Area: shell completions labels Sep 14, 2024
@ehuss ehuss added the A-testing-cargo-itself Area: cargo's tests label Sep 14, 2024
bors added a commit that referenced this issue Sep 14, 2024
Disable the shell_completions tests

The shell_completions tests have a few issues that are causing some problems. See #14545 for a description. This disables the tests until those can get resolved.
epage added a commit to epage/cargo that referenced this issue Sep 16, 2024
@epage
Copy link
Contributor

epage commented Sep 16, 2024

Deprecation: the legacy temporary assignment syntax is deprecated; use "tmp" instead

This was deprecated in 0.18 which is also when tmp was added. That is annoying for people installing from distributions as my distribution is still on 0.17.

@epage
Copy link
Contributor

epage commented Sep 16, 2024

The tests were added in #14493 for #14520. As our current completions do not have tests, not having tests for these new completions is on-par. As discussed in #14493 (review), the primary focus for the tests that call shells is to make sure the integration of clap_complete into cargo works. For everything else, we should either test the functionality directly, without a shell, or delegate the testing to clap_complete.

This could mean we drop the number of shells we verify in CI. If one works, they should all work. That could speed things up, reduce demand for custom tools, and possibly we can pick one that does a good job of threading the different failure cases.

@epage
Copy link
Contributor

epage commented Sep 16, 2024

To add to that, we also started this experiment with enabling all shells clap_complete supports. We can easily narrow those down to parity or maybe a step above parity. We have that being tracked in #14520

bors added a commit that referenced this issue Sep 25, 2024
test: Remove completion tests

The tests are intended to spot check that shell completions are registered correctly.  That is a low change, low risk area.  For shell integration, we're relying on `clap_complete`s tests. For our own candidates, we should test the candidate generation directly, rather than end-to-end.

This reverts parts of commit e7ca9be, reversing changes made to bd5f32b.

Fixes #14545
@bors bors closed this as completed in 023f4c6 Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-completions Area: shell completions A-testing-cargo-itself Area: cargo's tests C-bug Category: bug S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
2 participants