search

rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.76k stars 2.42k forks source link

shell_completions test issues #14545

Closed ehuss closed 1 month ago

ehuss commented 1 month ago

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. https://github.com/rust-lang/cargo/pull/14541#issuecomment-2351096767 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)
  4. 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.
  5. 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 643a025b3c3ad6f7d3acea558d223784ea8ab932 of master.

epage commented 1 month ago

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 commented 1 month ago

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 https://github.com/rust-lang/cargo/pull/14493#pullrequestreview-2280762279, 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 commented 1 month ago

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