Repository navigation
RFC: Make infection easier to debug #1111
Description
At its core I find infection quite hard to debug for a number of reasons, here is a non-exhaustive list
- We have quite a few debug/verbosity options:
--debug--log-verbosity--verbose
- It is not always clear what those debug/verbosity options are about and it's really frustrating to not being able to get the information you want after using one option and realising you missed another one
- When the temp folder is kept, you still need to look it up
- You have minimal information displayed to the user, e.g. of informations that could be displayed:
- the configuration file used
- if the server is going to be restarted
- what code coverage generation tool is going to be used
- where the coverage has been generated or with what command
- what is being logged and where
I also aim to remove the need to require the configuration file in the future. For now those efforts are halted because of Configuation::getSrcDirs() used for Codeception (/cc @maks-rafalko). However even if resumed, it's imperative to have a way to know what was the config values. We already have a few "inferred" values, which are resolved between the config file and the command line options, and with a configuration-less config, there's only going to be a lot more of them. I think I could take from the approach I used with Box for that.
I would like to work on the different points above in the near future. I don't think there is much that is going to cause controversy in there, except for one main point which I would like to discuss now with you.
I think we should drop the idea of dumping temporary files in a temporary directory. Instead I would propose to dump everything in a .infection directory. The benefits are:
- the location is easy to know/see and pick up
- by picking the same location between two runs it's easier to ensure we're not leaving too many artefacts behind (they will always be in the same directory, so easier to clean up)
- it makes any artefact, if we desire to not delete them (e.g. in debug mode), easier to inspect and use as a consumer
A downside is that it requires users to add .infection to their .gitignore, but given that PHPUnit itself recently requires so with the cache file, I think it's an acceptable trade-off. WDYT?
Activity
maks-rafalko commentedon Mar 4, 2020
Random thoughts
tmpDirconfig setting insideinfection.jsonto achieve it?tmpDirsetting).infectionthe best name/tmpfolder AFAIK is getting cleared automatically, which is a benefit since Infection produces a lot of files (hundrends of Megabytes or even Gigabytes). No automatic cleaning will be done with.infectionAnd the main (from my POV) point is
tmpfsfor/tmp, so basically all the files are stored in RAM. It's for sure speeds the Infection process up, which is a good thing for our users.theofidry commentedon Mar 4, 2020
Yes totally, I was thinking of something along the lines of making it point to
.infectionby defaultNot sure to understand the comment: dot files/directories are a common pattern. We already have
.github,.composerand.ciin infection itself. PHPUnit.phpunit.result.cache, PHP-CS-Fixer.php_cs.{dist,cache}, git.git,.gitattributes... Dot files are not going anywhere. Happy to take a suggestion, but for a debug artefact this looks appropriateI guess it gets cleared eventually yeah. But it's not immediate either, so in any case we need to clean up after a run.
My main beef here is UX: you have the logs giving you the commands, but can't use them easily because the files are gone.
But maybe this does not need to be systematic and could be done only with
--debug: so that you don't have to fetch them from a weird tmp folder and can more easily inspect the content.Also in case it wasn't clear: I don't mean we keep everything between two runs: a new run will wipe out the content of
.infectionthat was previously left if existed.But those tmp files are not meant to be re-used or inspected by an end-user like in our case. I'm not following the RAM issue though
maks-rafalko commentedon Mar 4, 2020
command line is added to log file only with
--debug, and files are not removed only with--debug. Isn't it a case?I'm not sure I got it. RAM is not an issue. Using
/tmpwhich is attached totmpfsis faster than using a disk, this is what I wanted to say.Anyway, if users want to inspect the files, they need
--debugHaving them in
.infectionwon't make things easier - files still won't be stored there by default (they will be removed without--debug) and users will not know that.infectionis the default folder.Probably, as an idea, we can add a question to ConfigureCommand to ask people where they want to locate files, as well as providing info that files are stored only with
--debug. We can even update.gitignoreif the user chooses./.infectionin the same way as Symfony Flex does it in its recipes.sanmai commentedon Mar 5, 2020
Let's think you want Infection to make a report for users to copy-n-paste, in case, for example some unknown error happens, or some fairly common error like "class not found" when coverage files are out of sync.
And therefore you would want to accept and handle all exceptions on one place. If you think about, it'll make many difficulties we have with error handling and exceptions (it appears #1107 is there to work around them) much less of a problem. Say, now you have a
CoverageDoesNotExistException, and it isn't just an exception, it's an exception with a help article attached to it. Why should it care? Is this a job for an exception to handle help articles?.. I'm not convinced. It isn't only exceptions. For example,PhpUnitXmlCoverageFactorynow has to know which test framework we use, for no other reason than to include this information with an exception.With all fairness, this is a valid approach when you want things done, but if you'd have every exception or error handled in one place, you can handle related tasks right here too. Say, what to show a user if a certain exception happened? You know it, and you have all the context, name of the framework, files, locations, etc. An exception object doesn't even need to care.
maks-rafalko commentedon Mar 5, 2020
@sanmai I'm sorry I've read it 3 times and even thought it was posted to not correct issue
Could you please provide any examples supporting your message? I don't follow how this RFC solves what you described.
Aren't we discussing just moving log files from
/tmpto.infection? If so, it's not related to all those exceptions and to #1107[-][RFC] Make infection easier to debug[/-][+]RFC: Make infection easier to debug[/+]