-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixes to make mutmut3 work with inline-snapshot #338
base: main
Are you sure you want to change the base?
Conversation
mutmut/__main__.py
Outdated
@@ -683,6 +687,8 @@ def new_tests(self): | |||
class PytestRunner(TestRunner): | |||
def execute_pytest(self, params, **kwargs): | |||
import pytest | |||
params+=["--rootdir=.","--inline-snapshot=disable"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This seems a bit weird. Do we need some config option in mutmut for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe, I think it should not be required and it could also be a problem in inline-snapshot. But this is currently useful to make things work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --rootdir was important. I had several problems to convince mutmut to test the code under mutants/
mutmut/__main__.py
Outdated
output_catcher.dump_output() | ||
print(f'failed to collect stats. runner returned {collect_stats_exit_code}') | ||
exit(1) | ||
collect_stats_exit_code = runner.run_stats(tests=tests) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use outputcatcher.stop() to stop it from swallowing output. This is a better method for debugging I find.
Although, the real solution is to add a flag or config to turn it off globally I guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the Tip
4ef49cf
to
a813871
Compare
@boxed this is where I'm currently:
I have no Idea why it does not test the mutations. |
I fixed some src-layout related issues (I dont know if I fixed them in the right way, but It works for me for now). ... something for another day. @boxed maybe you can use the fixes here as a base for some real fixes. I don't understand your whole code base and don't know where these things should be fixed. |
Cool! Yea that's great! Timeout for mutants that introduce infinite loops isn't implemented yet. I just disabled break->continue mutation and similar to avoid the scenario as far as possible without timeout support. But I suspected that this won't work 100% and unfortunately this happened sooner than I expected 🤣 |
nice, I got the memory limit working. The last open problem is propably my own pytest plugin which comes with inline-snapshot. |
Memory limit is an interesting idea. You could set it at like 20% over the current memory, as the forked process should have loaded 100% of what is needed from the parent clean test run. That could probably ship in mutmut as is. |
The limit is per process. I experienced that the memory usage got much higher than the limit because multiple tests where using a lot of memory at the same time.
But there is some memory which gets freed, or? and what is if the application has only one test. 20% memory would not be enough in this case |
Yea you're right. It should be x% over of the peak memory usage during the clean run, which is probably hard to capture correctly. I'm used to working with systems where the import graph dominates the memory usage, but you're right in that this is not generally true. |
this is a bit off topic but you might like https://github.com/15r10nk/lazy-imports-lite if you have a lot of imports 😃 |
Ooh. That is some dark magic 🤣 Cool! |
Do you have any idea how you could limit the time which is used to test a mutant? I think my tests getting slower because some of the parallel mutant tests are running forever. I was also never able to finish the mutant tests. |
I get so far and the progress stops then. ⠸ 1817/1829 🎉 1498 🫥 209 ⏰ 0 🤔 0 🙁 110 🔇 0 |
I think the idea forward is to implement something similar to how mutmut2 worked: time the full test run, and forcibly kill all workers that have run longer than the full time * 1.5 or something like that. There was some rule to have a minimum time added before the timeout value was implemented to handle really small test suites, but that was mostly it I think. |
but the problem is that the whole thing can block if you limit the number of parallel processes to 15 maybe and 15 process decide to never terminate. Mutmut will not be able to handle the remaining mutations any more. What about this:
|
That's what I was talking about yea. The master process that forks off children will never run mutation. It should store start time for each PID, and if a child has lived longer than the timeout it sends kill -9 to it and marks it as timed out in the result. |
I have fixed some src-layout related issues in my branch but I have still problems with I have the assumption that this caused by some general bug how some keys are generated or so. |
Ah I think you need to delete the mutants folder and try again. The name mangling was changed and you have the old stuff on disk. |
I delete it everytime before I run |
I pushed a few of fixes on main. |
218fc18
to
d9a5ba8
Compare
resource.RLIMIT_CPU looks very interesting as a way to implement timeout totally in the worker process. |
|
Nice. I looked into syntax highlighting, but I failed at finding it heh |
It's weirdly recollecting stats every single time. So something is wrong there. In any case, I got it to run through the entire test suite (if I rm mutants/ before)!
all changes for that are on main. |
d9a5ba8
to
bc308a1
Compare
I squashed everything into 2 commits. The fixes and the diff-syntax support. |
damn, mutmut is not deterministic (at least not for inline-snapshot) I run ⠋ 1829/1829 🎉 1355 🫥 209 ⏰ 0 🤔 0 🙁 265 🔇 0 |
Well that's interesting to say the least! |
I get this a lot:
Maybe the memory limit idea isn't good? The resource CPU time limit should cover this problem better no? Because if the process starts allocating a lot and that starts to swap and stuff it will hit the CPU limit pretty fast anyway. Or am I not understanding the issue? |
I cherry picked the syntax highlighting fix into |
I released 3.2.0. This version runs through
It does weirdly print a bunch of output for some failed mutants, which I don't understand. |
The reason for the output is that inline-snapshot comes with a pytest plugin which gets also mutated during test. |
bc308a1
to
65737b2
Compare
Some of my fixes are still necessary for inline-snapshot. This is what I get with the current main:
inline-snapshot runs some tests in a subprocess where mutmut is not initialized. |
Ah. Crap. The reason for that is that I have that setting in my conf locally. My bad. |
No, that's not it. I misremembered. Then I don't understand. |
Well this crash may be a hitbug then. If we make I don't know what the solution to that is. I don't want cross-process communication for this if it can be avoided. |
The mutliprocess support of coverage.py uses different coverage files per process. Could mutmut use a seperate file The location of these files should be derived from an env variable because the process can have a different working directory. |
I would be worried about pid based filenames about reuse. Maybe an env variable could give a full path to the file and the master process decides the name to avoid collisions. |
mutmut/__main__.py
Outdated
@@ -957,36 +957,59 @@ def should_ignore_for_mutation(self, path): | |||
return False | |||
|
|||
|
|||
@lru_cache() | |||
def read_config(): | |||
def config_reader(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mean it ONLY reads from pyproject.toml and now ignores setup.cfg? That's no good imo. If mutmut should force pyproject.toml it should at the very least error out if there is a mutmut section in setup.cfg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, the config reader is your old s()
function. It returns the old setup.cfg
reader if there is no pyproject.toml with an [tool.mutmut]
key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I cherry-picked this fix into main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also cherry-pick my fix for the debug option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There can never be two subprocesses with the same pid at the same time. |
Ah. True. If it just reads before writing then pid reuse would be totally fine. Maybe even better. |
I added a |
09065b9
to
be0e4e6
Compare
be0e4e6
to
de58b19
Compare
this is currently all work in progress ...
it is important that the actual inline-snapshot package is NOT part of the venv.
steps to create the venv:
... inside inline-snapshot repo using the mutmut3 branch