Skip to content

test(pypi): add argparse.bzl tests#3697

Merged
rickeylev merged 1 commit intobazel-contrib:mainfrom
aignas:aignas.tests.arparse-tests
Apr 12, 2026
Merged

test(pypi): add argparse.bzl tests#3697
rickeylev merged 1 commit intobazel-contrib:mainfrom
aignas:aignas.tests.arparse-tests

Conversation

@aignas
Copy link
Copy Markdown
Collaborator

@aignas aignas commented Apr 12, 2026

This simply adds extra unit tests to ensure that the code I have added in #3691
is well covered.

@aignas aignas requested review from groodt and rickeylev as code owners April 12, 2026 10:19
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +8 to +18
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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)

Comment on lines +20 to +29
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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)

Comment on lines +31 to +40
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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)

@rickeylev rickeylev enabled auto-merge April 12, 2026 20:26
@rickeylev rickeylev added this pull request to the merge queue Apr 12, 2026
Merged via the queue into bazel-contrib:main with commit fa783be Apr 12, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants