NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.41k stars 14.36k forks source link

Deselect specific test case in specific file using pytestCheckHook #118770

Closed mweinelt closed 11 months ago

mweinelt commented 3 years ago

Problem

We currently offer disabledTestPaths and disabledTests for the pytestCheckHook.

This is a very convenient way to disable tests that aren't really working for us for one reason or another.

Now a new problem appeared for me in home-assistant. One of the components tries to connect out to the network in test_form_cannot_connect. Now normally you would go and

  disabledTests = [ "test_form_cannot_connect" ];

but there are over 50 test functions with that same name!

Solution

Enter the --deselect commandline, which offers a more granular option to disable test cases, down to a specific file containing the test. So what I currently do is

  pytestFlagsArray = [
    "--deselect tests/components/screenlogic/test_config_flow.py::test_form_cannot_connect"
  ];

Question

Would it preferable to allow these kind of qualifiers in disabledTests and filter them based on whether they contain ::, which cannot occur in function names, and pass them to --ignore and --deselect respectively?

Or would we rather want to have another dedicated list for that use case?

In general I think qualifying the file in which the test occurs might be the smart thing to do in most cases, to reduce ambiguity.

Maintainers

@SuperSandro2000 @jonringer @FRidh

SuperSandro2000 commented 3 years ago

Is there a need for this option outside of home-assistant? I can't remember seeing deselect anywhere in nixpkgs yet and right now we would just disable all tests with that name ignoring the extra ones.

Can deselect take other arguments which we would want to use? If not I think we could add it to disabledTests to reduce the extra variables you can possibly define. It should be easy to detect of we can use new test with regex compare. [[ $var =~ :: ]]

mweinelt commented 3 years ago

Is there a need for this option outside of home-assistant? I can't remember seeing deselect anywhere in nixpkgs yet and right now we would just disable all tests with that name ignoring the extra ones.

It is unclear to me how many tests we have accidentally disabled, because they share the same name, but there is certainly n > 1 within nixpkgs.

People most likely jump onto the disabledTests or, before that existed, the -k test_foo bandwagon without thinking too much about the possible side-effects.

jonringer commented 3 years ago

People most likely jump onto the disabledTests or, before that existed, the -k test_foo bandwagon without thinking too much about the possible side-effects.

Which is still a significant improvement from no tests.

However, I think it would be nice for upstream to support an offline use case with markers. Or another option would be a way to fetch the test resources before running the tests.

Also, we can always just revert back to using pytest with checkPhase. In most cases, the current pytestCheckHook is more than sufficient.