Add validators for 'python_major' and 'python_minor' arguments

[lnxpy]

• argparse validators implementation
• add cli tests

[codecov]

Codecov Report

Merging #402 (d71797f) into main (a5889af) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #402      +/-   ##
==========================================
+ Coverage   99.60%   99.62%   +0.01%     
==========================================
  Files           8        8              
  Lines         764      792      +28     
==========================================
+ Hits          761      789      +28     
  Misses          3        3              

Flag	Coverage Δ
macos-latest	99.62% <100.00%> (+0.01%)	⬆️
ubuntu-latest	99.62% <100.00%> (+0.01%)	⬆️
windows-latest	99.62% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/pypistats/cli.py	98.98% <100.00%> (+0.16%)	⬆️
tests/test_cli.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[hugovk]

Please could you add tests in https://github.com/hugovk/pypistats/blob/main/tests/test_cli.py?

[lnxpy]

Hi @hugovk. Would you work on the remaining tasks?! They look a bit confusing to me actually. Sorry for that.

[hugovk]

Sure! Let me explain the general idea:

@c("test_input", ["2", "3", "4"])
def test__python_major_version_valid(test_input: str) -> None:
    # Act / Assert
    assert cli._python_major_version(test_input) == test_input

assert cli._python_major_version(test_input) == test_input is the main bit. We're passing an input value to the function, and asserting the output is as expected. In this case, the input should be valid, so the output should be the same as the input.

@pytest.mark.parametrize("test_input", ["2", "3", "4"]) is a nice bit of magic from pytest.

It runs the test function three times for each input value there: "2", "3" and "4". Each time is like a fresh run, independent of any previous inputs. It's a nice way to repeat a test with the same logic but with different input values. It's basically a do-not-repeat-yourself of doing:

def test__python_major_version_valid2() -> None:
    # Act / Assert
    assert cli._python_major_version("2") == "2"

def test__python_major_version_valid3() -> None:
    # Act / Assert
    assert cli._python_major_version("3") == "3"

def test__python_major_version_valid4() -> None:
    # Act / Assert
    assert cli._python_major_version("4") == "4"

We could of course loop it:

def test__python_major_version_valid() -> None:
    # Act / Assert
    for test_input in ("2", "3", "4"):
        assert cli._python_major_version(test_input) == test_input

But if it failed for, say, "3", then it wouldn't run it for "3". pytest.mark.parametrize runs each one separately, so even if "3" fails, it will run "4".

[hugovk]

The invalid one is similar:

@pytest.mark.parametrize("test_input", ["2.7", "3.9", "3.11", "-5", "pillow"])
def test__python_major_version_invalid(test_input) -> None:
    # Act / Assert
    with pytest.raises(argparse.ArgumentTypeError):
        cli._python_major_version(test_input)

Except that _python_major_version won't return a value for invalid input, so we don't assert the output is a value, we use the pytest.raises context manager to check the mentioned exception is raised.

Then we have a similar pair for _python_minor_version.

[hugovk]

Would you to look at exception handling for empty data or shall I check that?

We could actually merge this PR as it is, it's nicely self-contained, and do that bit in another PR.

[lnxpy]

I actually thought that you want me to add tests for the actual CLI arguments like the args.python_major, args.python_minor, and args.system. I could add tests for that small validator piece. Thanks for your complete explanation though. :))

[lnxpy]

Yes indeed. We can have that exception handling in another PR. I unlinked this PR. By merging this PR, the issue won't get closed anymore. Would you please work on that part too?! I looked over the codebase days ago and that was a bit hard to figure out its parts. Keep in mind that I looked over it with the mindset that I talked about in #401.

For the sake of simplicity, we can have some refactoring and improvements next up. 🍻

[hugovk]

Sounds good 👍