feat: Support Enum, @staticmethod, @classmethod mutations#476
feat: Support Enum, @staticmethod, @classmethod mutations#476nicklafleur wants to merge 8 commits intoboxed:mainfrom
Conversation
c701b42 to
0ab4303
Compare
072319e to
b6c5fa3
Compare
|
Hi, thanks for the PR! I'll take a look this week, not sure yet when I'll find the time.
Can you explain which bug this solves? How did the pid lookup become stale (did someone else modify the dict in the same time?) and how does this cause a hang? |
Hey sorry I should have included more details in the PR, here's what I added in the commit 40eb31d. |
Otto-AA
left a comment
There was a problem hiding this comment.
Thank you for the PR. I looked through it and it looks pretty good :)
My main open questions are in regard to the trampoline setup:
- why do we define a method multiple times? Once with the original signature, once with
*args, **kwargsand then again with@wraps? - Could we always define the mutant dict outside the class and the mutated methods inside the class? Would be nice if we could use the same structure instead of special casing enums and staticmethod/classmethods
Judging from the E2E tests, it looks pretty backwards compatible, except for the type checking feature. Maybe I get to test the PR changes tomorrow on a small project.
I could imagine that in the short term we can add # type: ignores to mutated @staticmethod and @classmethod methods, if it's hard to get that right with typing. However, I'd prefer if the old E2E tests keep working.
tests/mutation/test_mutation.py
Outdated
| try: | ||
| os.environ["MUTANT_UNDER_TEST"] = "none" | ||
| namespace = {"__name__": "test_module"} | ||
| exec(mutated_code, namespace) |
There was a problem hiding this comment.
I think I'd prefer the exec integration tests to be in another file. They are somewhere between the tests in this file and E2E tests.
There was a problem hiding this comment.
good point, will do
|
Regarding
If we do something like #477 , we would have access to |
nicklafleur
left a comment
There was a problem hiding this comment.
Thanks for the review, I'll take care of the changes in the coming days.
06f126b to
7707f44
Compare
|
@Otto-AA sorry about the long turnaround on this, ended up finding a bug with how the external mutation handled forward reference type-hints that needed to be fixed before pushing. Also spent a little time doing type-fu to get the trampolines to properly display types in IDEs, admittedly didn't look too much into how this could be used to augment the type-based mutant invalidation as this PR is already big enough. I tried to separate the moved stuff from |
0db5873 to
c6021bf
Compare
|
Thanks for the updates! I don't think I'll have time to review this weekend, but hopefully in the course of the next week. |
no problem, I'm still polishing things a bit today anyways so I'll happily take the time |
f45e35a to
d9590d9
Compare
Changes: - Parametrizes the dockerfile and script so that tests can be run against any arbitrary python version. - Improves dockerfile so that they can be better cached between runs
d9590d9 to
7f85db6
Compare
- Extract Config class to dedicated src/mutmut/config.py module with
singleton pattern (Config.get(), Config.reset(), Config.ensure_loaded())
- Replace all mutmut.config global references with Config.get() calls
- mutmut.config global was kept, but put behind a deprecation warning
- Add type annotations throughout codebase to satisfy mypy strict checking
- Add safe_setproctitle wrapper to handle macOS Python 3.14+ fork crashes
where setproctitle's CoreFoundation usage causes segfaults after fork()
- Fix various type: ignore comments with proper casts and annotations
- Update tests to use new Config API
Bug Fix:
The old timeout_checker used the loop variable `mutant_name`
from the outer mutants iteration when looking up estimated_time_of_tests,
instead of looking up the mutant name associated with each PID, causing
some tests to hang and never be killed due to incorrect timeout
calculations.
This was easy to miss because the bug only manifested when multiple mutants
were being tested in parallel and the timing was just right for the wrong
mutant's timeout to be checked against another mutant's PID (in my case,
it only happened specifically when running the e2e tests in vscode
after my last set of changes 🤷)
Instead of trying to patch this specific bug (and probably introducing
one), I decided to go with a simpler design that avoids trying to find
the right pid at all, instead using a min-heap to track (timeout, pid)
tuples.
Flow:
- Registers timeouts at fork time with register_timeout(pid, timeout_s)
- Calling this function lazily starts the timeout checker thread if not
already started
- Uses a min-heap to track the next deadline
- Processes expired timeouts in order, sending SIGXCPU to each PID
whose deadline has passed
- If (when) a PID exits before its timeout, its entry remains in the heap
until it reaches the top, at which point we pop it and try to kill it
with SIGXCPU (same as before), swallowing the ProcessLookupError that
is raised if the PID is already gone.
- a process hanging indefinitely due to a mutation will back up the
heap with stale entries, but each entry is small (~72 bytes) so
even 10,000 backed up timeouts before it gets killed is less than
1MB of memory and saves a O(n) rebuild for each backed up timeout
compared to cancelling.
- Clean up START_TIMES_BY_PID_LOCK since it's no longer needed
Deprecation Warning:
mutmut.config global is deprecated, use mutmut.config.Config.get()
instead
…unction Features: - Add enum class detection and external injection pattern for enum mutation - Add staticmethod/classmethod support via external trampoline pattern - Add parse_pragma_lines() for pragma: no mutate class/function - Add build_enum_trampoline() template Refactoring: - Extract pragma_handling.py: parse_pragma_lines() - Add utils/format_utils.py: make_mutant_key(), parse_mutant_key() - Simplify orig_function_and_class_names_from_key() using parse_mutant_key() Tests: - Add test_enum_handling.py mirroring enum_handling module - Add test_pragma_handling.py mirroring pragma_handling module Config: - Exclude AUTHORS.rst from merge conflict check in pre-commit
7f85db6 to
5c30455
Compare
Otto-AA
left a comment
There was a problem hiding this comment.
Looks good, just one issue remaining with the is_mutated_method_name for type checking. For now, I don't mind if @staticmethod or @classmethod will get "caught" accidentally by the type checker (e.g. because the cls parameter won't make sense outside of the class body), but it should be able to find the mutated methods when type errors occur.
I will have some time on the weekend, so I'll probably merge and make some small adjustments afterwards for type checking.
|
|
||
| for i, mutant in enumerate(mutants): | ||
| mutant_func_name = f"{prefix}_mutant_{i + 1}" | ||
| full_mutant_name = f"{mangled_name}_{i + 1}" |
There was a problem hiding this comment.
Is there a reason we use a different naming scheme for these external mutated methods? If yes, we need to modify the is_mutated_method_name method s.t. type checking can match type errors in mutated methods to the mutant names. Could be E2E tested by adding a @staticmethod to the type_checking tests.
There was a problem hiding this comment.
probably just moved a little too fast on this one and didn't consider the side-effects. I'll tweak this to make it work with the type checking mechanisms tomorrow/Saturday.
…pport Replace the text-based `parse_pragma_lines` with a LibCST visitor (`PragmaVisitor`) that operates on the parsed syntax tree, fixing edge cases with triple-quoted strings and multi-line constructs. Unify `# pragma: no mutate class` and `# pragma: no mutate function` into the more general `# pragma: no mutate block`, which works on any compound statement. Add `# pragma: no mutate start/end` for suppressing mutations across arbitrary line ranges. Also fix the `tests_for_mutant` CLI command which passed a bare string instead of a list to `tests_for_mutant_names`, and standardize docstrings to a consistent format.
afdf19c to
283bd63
Compare
| return visitor.no_mutate_lines, visitor.ignore_node_lines | ||
|
|
||
|
|
||
| class TestParsePragmaLines: |
There was a problem hiding this comment.
There's like 3 or 4 different stylings of source going on here.
This one looks like the easiest to read except for the \n characters:
source = (
"class Skipped: # pragma: no mutate block\n"
" def method(self):\n"
" return 1 + 1\n"
"\n"
"def skipped_func(): # pragma: no mutate block\n"
" return 2 + 2\n"
"\n"
"def mutated():\n"
" return 3 + 3 # pragma: no mutate\n"
)
Maybe this is better:
source = """
class Skipped: # pragma: no mutate block
def method(self):
return 1 + 1
def skipped_func(): # pragma: no mutate block
return 2 + 2
def mutated():
return 3 + 3 # pragma: no mutate
"""
There was a problem hiding this comment.
yeah good call, ended up manipulating stuff for testing a bit so things got weird but should have standardized it afterwards
| """Tests for # pragma: no mutate block.""" | ||
|
|
||
| def test_own_line(self): | ||
| source = "if condition:\n # pragma: no mutate block\n x = 1\n y = 2\nz = 3\n" |
There was a problem hiding this comment.
| source = "if condition:\n # pragma: no mutate block\n x = 1\n y = 2\nz = 3\n" | |
| source = """ | |
| if condition: | |
| # pragma: no mutate block | |
| x = 1 | |
| y = 2 | |
| z = 3 | |
| """ |
Same for all these below
515ed7c to
f54e066
Compare
|
@Otto-AA The pragma rewrite has cause this PR to balloon considerably beyond what I would generally be comfortable submitting, if you would like me to split the enum/classmethod/staticmethod support from the pragma features just let me know and I'll do so to make it reviewable. |
src/mutmut/mutation/file_mutation.py
Outdated
| if isinstance(updated_node.annotation, (cst.SimpleString, cst.ConcatenatedString, cst.FormattedString)): | ||
| return updated_node | ||
| source = self._empty_module.code_for_node(updated_node.annotation) | ||
| return updated_node.with_changes(annotation=cst.SimpleString(f'"{source}"')) |
There was a problem hiding this comment.
This breaks annotations that contain double quotes, for example:
Literal["one", 'two']
There was a problem hiding this comment.
Good catch and thanks for the additional test case!
There was a problem hiding this comment.
Thank you for the fast response and fix! 🚀
Mix of boy-scouting and new features along with docker improvements for non-linux systems.
Summary