-
Notifications
You must be signed in to change notification settings - Fork 93
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
test(stryker): Replace line numbers by error index #201
Conversation
so the snapshots match even when stryker mutates the files.
e28e57b
to
894d5a0
Compare
since stryker tends to put code in front of them. (Latest version of test/error/reported.js` deals with those anyways, but I don't see a reason why we should ship those lines downstream.)
Thanks for your timely work on this. My one, minor question is that it looks like this is using a possibly hackish utility function, with "sax.js" hardcoded multiple times. And I am not sure if I understand it 100%. I am thinking it may be ideal if we can find a better way to improve the reusability. |
I think I should be able to do another round of self review and try to refactor/document some more things to make them more generic & easy to understand. Of course the whole test suite about the reported errors and warnings is not trivial to understand. |
I think that is the key, and to be extra clear I think this is the result of the original design. Further refactoring and improvement would be awesome, assuming you have the time for it. Otherwise I would not object to improving it someday later. Thanks again for all of your care and efforts on such a messy code base! |
@brodybits I'm not sure what to understand from your reply. |
It is fine with me if you want to land these changes now. While I think it would be nice to land these changes in a better state, it would probably be better to fix the build now. Thanks again. |
@brodybits I think I did everything I could, to improve it in this short period of time. Please have a look, if this is the direction you were talking about. |
From a quick read it looks better to me, no more objections on my part. I would like to leave this up to @karfau to merge. I am also very happy to see the improvement in the mutation score. |
so the snapshots match even when stryker mutates the files.
The intermediate mapping is also stored to
tests/error/reported.json
for easier inspection, so I.gitignore
d it.The good news: The score improved quite a bit 🎉

Fixes #200