Repository navigation
TST [units]: restore consistent naming convention for array functions tests - #15719
Conversation
|
switching to draft while I fix failures |
b9d8117 to
8a55d07
Compare
neutrinoceros
changed the title
8a55d07 to
1a76000
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Looking at the actual changes, I think I'd prefer my earlier suggestion of a class attribute... See inline comments.
| raise RuntimeError(f"failed to match {k=}") | ||
| function_name = m.group("function_name") | ||
| if (module_name := m.group("module_name")) is not None: | ||
| module_name = "numpy." + module_name.replace("_", ".").lower() |
There was a problem hiding this comment.
I don't much like the captilization...
| def populate_covered_from_members(members, covered: set[FunctionType]) -> None: | ||
| for k, v in members.items(): | ||
| if inspect.isfunction(v) and k.startswith("test_"): | ||
| if (m := MOD_REGEXP.fullmatch(k.removeprefix("test_"))) is None: |
There was a problem hiding this comment.
This regex which forces the capitalization feels a bit over the top to me... How about we go back to my suggestion of a test class attritute, ie.,
module_name = members.get("module", "numpy")
(class attribute could also be "tested_module" for clarity, or something similar)
That means no changes to any of the test function names (except removing the two linalg ones).
|
|
||
| covered = set() | ||
|
|
||
| def __new__(mcls, name, bases, members): |
There was a problem hiding this comment.
Thought: if we change this to __init_subclass__(cls, tested_module="numpy", **kwargs), then we could have the signature of a test class be,
class TestFFT(InvariantUnitTestSetup, tested_module="np.fft"):
...
There was a problem hiding this comment.
This seems like a good idea, however I feel slightly out of my depth here, and I haven't succeeded in implementing it yet.
Could you show me how to do this change in a little bit more details, or maybe point me to relevant docs that I could use to learn about it ?
1a76000 to
80d8921
Compare
|
@mhvk I've rebased the branch and kept the newest changes as a separate commit. I really like your suggestion about changing the metaclass to avoid my inelegant case-based solution, but I think I'll need a hand to get it going ! |
|
@neutrinoceros - when looking at my suggestion, I realized Right now, it may be easiest to look at all our commits together; we should definitely squash them together when merging. |
|
Thank you so much @mhvk, I approve of your simplifications ! Now do we need someone else to bless this PR ? |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
@neutrinoceros - I think we can approve each other's contributions!
Description
Fixes #15692
ping @hamogu @mhvk