test(pypi): add argparse.bzl tests#3697
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new unit test suite for the pip argument parsing utilities, covering index_url, extra_index_url, and platform parsing. The feedback suggests adding a descriptive module docstring to the test file and expanding the test coverage to include edge cases such as null inputs, flag precedence where the last occurrence wins, the interaction between default values and command-line flags, and the deduplication of repeated flags.
| def _test_index_url(env): | ||
| env.expect.that_str(argparse.index_url([], "default")).equals("default") | ||
| env.expect.that_str(argparse.index_url([], None)).equals(None) | ||
|
|
||
| env.expect.that_str(argparse.index_url(["-i", "https://example.com/simple"], "default")).equals("https://example.com/simple") | ||
| env.expect.that_str(argparse.index_url(["--index-url", "https://example.com/simple"], "default")).equals("https://example.com/simple") | ||
| env.expect.that_str(argparse.index_url(["--index-url=https://example.com/simple"], "default")).equals("https://example.com/simple") | ||
|
|
||
| env.expect.that_str(argparse.index_url(["--extra-index-url", "https://extra.com", "-i", "https://index.com"], "default")).equals("https://index.com") | ||
|
|
||
| _tests.append(_test_index_url) |
There was a problem hiding this comment.
The test coverage for index_url can be improved by including cases for None as the args input and verifying that the last occurrence of the flag takes precedence when multiple are provided.
def _test_index_url(env):
env.expect.that_str(argparse.index_url(None, "default")).equals("default")
env.expect.that_str(argparse.index_url([], "default")).equals("default")
env.expect.that_str(argparse.index_url([], None)).equals(None)
env.expect.that_str(argparse.index_url(["-i", "https://example.com/simple"], "default")).equals("https://example.com/simple")
env.expect.that_str(argparse.index_url(["--index-url", "https://example.com/simple"], "default")).equals("https://example.com/simple")
env.expect.that_str(argparse.index_url(["--index-url=https://example.com/simple"], "default")).equals("https://example.com/simple")
# Test that the last occurrence wins
env.expect.that_str(argparse.index_url(["-i", "url1", "-i", "url2"], "default")).equals("url2")
env.expect.that_str(argparse.index_url(["--extra-index-url", "https://extra.com", "-i", "https://index.com"], "default")).equals("https://index.com")
_tests.append(_test_index_url)
| def _test_extra_index_url(env): | ||
| env.expect.that_collection(argparse.extra_index_url([], ["default"])).contains_exactly(["default"]) | ||
| env.expect.that_collection(argparse.extra_index_url([], None)).contains_exactly([]) | ||
|
|
||
| env.expect.that_collection(argparse.extra_index_url(["--extra-index-url", "https://extra.com/simple"], [])).contains_exactly(["https://extra.com/simple"]) | ||
| env.expect.that_collection(argparse.extra_index_url(["--extra-index-url=https://extra.com/simple"], [])).contains_exactly(["https://extra.com/simple"]) | ||
|
|
||
| env.expect.that_collection(argparse.extra_index_url(["--extra-index-url", "https://first.com", "--extra-index-url", "https://second.com"], [])).contains_exactly(["https://first.com", "https://second.com"]) | ||
|
|
||
| _tests.append(_test_extra_index_url) |
There was a problem hiding this comment.
It is important to document and test the behavior when both a default value and command-line flags are provided for repeated arguments. The current implementation appends to the default list, which should be explicitly verified.
def _test_extra_index_url(env):
env.expect.that_collection(argparse.extra_index_url([], ["default"])).contains_exactly(["default"])
env.expect.that_collection(argparse.extra_index_url([], None)).contains_exactly([])
env.expect.that_collection(argparse.extra_index_url(["--extra-index-url", "https://extra.com/simple"], [])).contains_exactly(["https://extra.com/simple"])
env.expect.that_collection(argparse.extra_index_url(["--extra-index-url=https://extra.com/simple"], [])).contains_exactly(["https://extra.com/simple"])
env.expect.that_collection(argparse.extra_index_url(["--extra-index-url", "https://first.com", "--extra-index-url", "https://second.com"], [])).contains_exactly(["https://first.com", "https://second.com"])
# Test interaction between default and provided flags (currently appends)
env.expect.that_collection(argparse.extra_index_url(["--extra-index-url", "url1"], ["default"])).contains_exactly(["default", "url1"])
_tests.append(_test_extra_index_url)
| def _test_platform(env): | ||
| env.expect.that_collection(argparse.platform([], ["default"])).contains_exactly(["default"]) | ||
| env.expect.that_collection(argparse.platform([], None)).contains_exactly([]) | ||
|
|
||
| env.expect.that_collection(argparse.platform(["--platform", "manylinux_2_17_x86_64"], [])).contains_exactly(["manylinux_2_17_x86_64"]) | ||
| env.expect.that_collection(argparse.platform(["--platform=manylinux_2_17_x86_64"], [])).contains_exactly(["manylinux_2_17_x86_64"]) | ||
|
|
||
| env.expect.that_collection(argparse.platform(["--platform", "macosx_10_9_x86_64", "--platform", "linux_x86_64"], [])).contains_exactly(["macosx_10_9_x86_64", "linux_x86_64"]) | ||
|
|
||
| _tests.append(_test_platform) |
There was a problem hiding this comment.
The implementation of _get_pip_args includes logic to deduplicate repeated flags. This behavior should be covered by a test case.
def _test_platform(env):
env.expect.that_collection(argparse.platform([], ["default"])).contains_exactly(["default"])
env.expect.that_collection(argparse.platform([], None)).contains_exactly([])
env.expect.that_collection(argparse.platform(["--platform", "manylinux_2_17_x86_64"], [])).contains_exactly(["manylinux_2_17_x86_64"])
env.expect.that_collection(argparse.platform(["--platform=manylinux_2_17_x86_64"], [])).contains_exactly(["manylinux_2_17_x86_64"])
env.expect.that_collection(argparse.platform(["--platform", "macosx_10_9_x86_64", "--platform", "linux_x86_64"], [])).contains_exactly(["macosx_10_9_x86_64", "linux_x86_64"])
# Test deduplication
env.expect.that_collection(argparse.platform(["--platform", "p1", "--platform", "p1"], [])).contains_exactly(["p1"])
_tests.append(_test_platform)
This simply adds extra unit tests to ensure that the code I have added in #3691
is well covered.