✅ Fix tests for completion flags

[rokbot]

--show-completions
zsh, fish, pwsh users could run tests with their default shell

--install-completions
(WIP: missing fish, pwsh)
this test is skipped due to side-effects

[svlandeg]

Hi!

Thanks for your contribution! Could you explain in a bit more detail the changes you've made to the tests, and why?

I also noticed that the tests are currently failing, so I put this PR in draft node. Do you have time to try and get these fixed so that the test suite is green again? This will make the review process go more smoothly.

[rokbot]

Hi! @svlandeg

Sure, this was an attempt to fix, simplify and improve the (2) failed tests under tests/test_completion/test_completion.py marked with the decorator @needs_linux (test_show_completion, test_install_completion) when running pytest directly without using bash scripts/tests.sh under a default shell different from bash, the SHELL environment variable make this test to fail when the current shell isn't bash. The conclusion i come to is to delete these (2) flaky tests earlier mentioned, they seems to be already covered in tests/test_completion/test_completion_show.py and tests/test_completion/test_completion_install.py.

If you agree i will update the PR to reflect these changes.

[svlandeg]

the SHELL environment variable make this test to fail when the current shell isn't bash.

You're right - the @needs_linux tag is not perfectly suited because a different shell can be used on Linux, and vice versa this test succeeds in Bash on Windows. So, what I suggest we do here instead is to write a new tag, something like @needs_bash, and decorate these two unit tests with that instead. Then we don't have to touch anything else in the unit test framework - in general I'm hesitant to remove/reduce tests, so I'd prefer a minimal change instead.

Let me know if you have time to rework the PR accordingly - if not I can also have a look.

[rokbot]

Hello @svlandeg, hope you are doing well!

Sure we can go with the approach you mentioned, but while I'm with you with the idea of not removing o reducing the test suite, I think the unit tests must be useful and test something, be fast and simple and contribute with the project coverage to give a degree of confidence, not just be there to "have tests", in this particularly case, these unit tests only were executed "on linux", now will be "needs bash" to me this is just a slow deprecation cycle that leads to "just delete this tests" at the end, to this (2) tests that are no longer useful to the project. Indeed these unit tests have dedicated modules that test it test_completion_show.py and test_completion_install.py respectively. I'm more worried about the side effects that the test suite leaves after executing it, to me the "tester" system must not be affected by the test suite, like the --install-completion tests, that make changes to your system and not clean after finish when you execute it. I'm don't have much time available now, so if you want to implement this sooner is fine to me. If i got some free time will work on this.

[svlandeg]

in this particularly case, these unit tests only were executed "on linux", now will be "needs bash" to me this is just a slow deprecation cycle that leads to "just delete this tests" at the end

Just to provide a bit more background: the needs_linux or needs_bash were not introduced as some sort of "slow deprecation". Instead, they were actually introduced to be able to keep these tests when we extended our CI to include Windows and MacOS testing. At the time, I noted that follow-up work could consider extending these tests to be less shell/OS-specific. It looks like you attempted to do this, but the test suite was still failing, probably because you didn't adjust the subprocess.run call that executes "bash".

Please don't get me wrong - I really appreciate the effort you've made into updating these tests and explaining why. It's just that from a maintainer's point of view, it's much easier to review atomic changes instead of 3 edits done for 3 different reasons - especially when the final PR is not passing the CI.

So, to ensure we follow up on this work properly and to capture some of the arguments you've made here, I started a new PR to first address the needs_bash change.

I'm more worried about the side effects that the test suite leaves after executing it, to me the "tester" system must not be affected by the test suite, like the --install-completion tests, that make changes to your system and not clean after finish when you execute it.

I definitely agree that we should look into the "side effects" caused by the test_install_completion() test - good point. I'll look into it and will likely open another PR later on.

Thanks again! 🙏

[svlandeg]

@rokbot: I looked into this more. Are you sure the current implementation of test_install_completion causes side effects? It has a line

bash_completion_path.write_text(text)

which should restore the original bash profile, so in fact it shouldn't have any side effects as far as I can see.

[svlandeg]

In the meantime, #888 has been merged, which adds the needs_bash functionality. I suggest that we close this PR and optionally follow up with atomic, separate PRs to address one issue at a time (and to ensure that the test suite succeeds for each change). Thanks again for the good discussion!

[tiangolo]

Thanks for the interest @rokbot! ☕

And thanks for the help @svlandeg! 🙇 Sofie, maybe we could have the "install completion" tests run only under some opt-in env var or something like that, that we could enable by default on CI, or we could enable manually when testing locally, but wouldn't affect others that come and try to run the tests. What do you think? 🤔 Anyway, it's also not urgent, but we can think if we can find a way to improve that process.

---

I'll close this PR now and we can continue later in subsequent PRs. 🍰

[svlandeg]

Sofie, maybe we could have the "install completion" tests run only under some opt-in env var or something like that, that we could enable by default on CI

Yea, that's a good idea! I'll look into it when I have some time in the next few weeks.