Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(stryker): Replace line numbers by error index #201

Merged
merged 3 commits into from Mar 23, 2021

Conversation

karfau
Copy link
Member

@karfau karfau commented Mar 21, 2021

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 .gitignored it.

The good news: The score improved quite a bit 馃帀
image

Fixes #200

@karfau karfau added the testing label Mar 21, 2021
@karfau karfau added this to the 0.6.0 milestone Mar 21, 2021
@karfau karfau requested a review from brodybits March 21, 2021 18:46
so the snapshots match even when stryker mutates the files.
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.)
@brodybits
Copy link
Member

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.

@karfau
Copy link
Member Author

karfau commented Mar 21, 2021

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.
But I'm not sure how fast I'll be able to do that.

Of course the whole test suite about the reported errors and warnings is not trivial to understand.

@brodybits
Copy link
Member

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!

@karfau
Copy link
Member Author

karfau commented Mar 22, 2021

@brodybits I'm not sure what to understand from your reply.
Do you think we can land it as is to solve the stryker issue and improve it later, or do do you prefer we invest more time into this before we land it?
(From my perspective, we should land it so we can take the next steps towards 0.6.0 .)

@brodybits
Copy link
Member

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.

@karfau
Copy link
Member Author

karfau commented Mar 22, 2021

@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.
You can land the PR or I will do it in some days, if you don't give me a different signal.

@brodybits
Copy link
Member

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.

@karfau karfau merged commit 5869d76 into xmldom:master Mar 23, 2021
@karfau karfau deleted the 200/fix-stryker-runs branch March 23, 2021 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stryker test failures on GitHub
2 participants