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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix empty part bug, 2nd try #55

Merged
merged 8 commits into from
Dec 2, 2021
Merged

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Dec 2, 2021

@ruur
@kibertoad

Lets talk about the empty part bug here.

I use a simpler test than in the original PR. As you can see, it doesnt throw an error, despite it should throw an "Uncaught AssertionError: Parsed result count mismatch. Saw 4. Expected: 3: expected 4 to deeply equal 3"

If you uncomment the part in Dicer.js, you will see that it will then throw the expected AssertionError. Then you uncomment that line in the unit test, and it will become green.

I could not create a red phase, because it is like @ruur described hanging. So if you confirm @kibertoad I would uncomment the commented out codes, and we should merge it.

Checklist

@Uzlopak Uzlopak mentioned this pull request Dec 2, 2021
4 tasks
@kibertoad
Copy link
Member

kibertoad commented Dec 2, 2021

@Uzlopak If I uncomment the test part, but do not uncomment the Dicer part, all tests still pass for me. It hangs for few seconds after execution, but still completes correctly.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 2, 2021

Exactly. It passes, despite it should fail. You have to uncomment the part in dicer to get the tests fail as expected.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 2, 2021

I use a red-green-blue approach. But here I dont get red phase, because the test itself cant handle the case.


I think we have to add done into mocha it and call done in the finish to be sure that we will hang.

@kibertoad
Copy link
Member

Wait, so is the commented part in Dicer a reproduction or a fix :D?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 2, 2021

A fix. Now I have my red phase... So I can be sure, that the bug fix actually has impact.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 2, 2021

@ruur any comments?
@kibertoad it should be fixed now.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 2, 2021

@kibertoad
modified other unit tests to callback style where necessary to ensure we dont missed a bug.

@Uzlopak Uzlopak merged commit a099c03 into fastify:master Dec 2, 2021
@Uzlopak Uzlopak deleted the fix-empty-part branch December 2, 2021 16:43
Copy link

@ruur ruur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants