Un-swap the pass_by_address parametrize ids in test_kernelParams - #2559
Open
LeSingh1 wants to merge 1 commit into
Open
Un-swap the pass_by_address parametrize ids in test_kernelParams#2559LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
@pytest.mark.parametrize("pass_by_address", [False, True], ids=["by-address", "not-by-address"])
pytest assigns ids to argvalues positionally, so pass_by_address=False gets
the id "by-address" and pass_by_address=True gets "not-by-address". The body
is unambiguous about which is which:
ctypes.addressof(packagedParams) if pass_by_address else packagedParams
True is the by-address case, so both ids name the opposite variant. Every
failure report, -k selection and flake attribution for this test points at
the wrong one. The neighbouring parametrize in the same file gets the
mapping right (`[False, True]` with `ids=["no-ctypes", "ctypes"]`).
Both branches still execute, so no code path was going untested; this is a
labelling fix only, which is why there is no new test -- pytest's id
assignment is what is being corrected, and `--collect-only` shows it
directly.
Applied to the legacy_api copy as well, so the two files stay consistent.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pytest assigns
idstoargvaluespositionally, sopass_by_address=Falsegets theid
by-addressandpass_by_address=Truegetsnot-by-address. The test body isunambiguous about which is which:
Trueis the by-address case. Both ids name the opposite variant.Confirmed with
--collect-onlyon a minimal reproduction of the same decorator shape:Impact is bounded — both branches still execute, so nothing goes untested — but every
failure report,
-kselection, xfail entry and CI flake attribution names the wrongvariant. The neighbouring parametrize in the same file gets the mapping right
(
[False, True]withids=["no-ctypes", "ctypes"]), which is what the fix matches.No new test
The thing being corrected is pytest's id assignment, so a test asserting the ids would
just restate the decorator.
pytest --collect-only -k kernelParams_buffer_protocolshowsthe change directly, and the repro above pins the mechanism.
On the
legacy_apicopycuda_bindings/AGENTS.mdsays the legacy tests "should not be added to, only updated whennecessary to fix test failures". I applied the same one-line change there so the two files
do not disagree about what
by-addressmeans — nothing is added, and a wrong label isarguably a failure-reporting defect. Say the word and I will drop that hunk and keep this
to the canonical file.