Conversation
|
Nice! |
|
I think this PR introduced a regression for default-value mutations. With the old *args, **kwargs trampoline, a mutation to a default parameter was observable because the mutant function resolved its own default. With the new signature-preserving trampoline, the wrapper resolves the default before dispatch, making the mutation unreachable. For example, given this source: The generated trampoline is: I'm new to mutation testing and to this repo, so let me know if I'm missing something. |
|
Thanks for pointing this out. I currently don't have the time to verify this, but it does look like a regression. I'm not sure yet how to fix it (removing the default value would break calls that do not pass the "fix" value; keeping the default would make it hard to know if a value has been passed or not; maybe we could set it to some custom unique value instead?) |
|
I've opened #477 to track this default arg issue |
Closes #454 and #465 .
@boxed In case you want to review, this is a slightly bigger change of trampoline internals. This PR adds some complexity at the trampoline wrapper (handling various possible parameter formats, async declarations and async generators), but I think it's worth it so we can preserve the original function signature. Also, don't need to manually copy the signature with
trampoline.__signature__ = inspect.signature(original_method)anymore.With this change,
asyncio.iscoroutinefunction(async_func)also returns true after we mutated the function. This is done by using the same signature as it was originally, and adding some logic to pass our arguments to the trampoline.For instance:
Will be mutated to:
In the commit 944d43f, I've also added
inline-snapshot(+ruff formatting for the snapshots) to make it easier to work with regression snapshots. e.g. thetests/test_mutation regression.pyare useful to have and withinline-snapshotit's easier to update them whenever some implementation detail changes.The e2e tests and a sample repository I tried were not affected by this change, so I don't think it breaks existing functionality.