Skip to content

Conversation

@DanielRuf
Copy link

No description provided.

@mscdex
Copy link
Owner

mscdex commented Apr 30, 2018

This is not a solution as this just hides an underlying issue with node (v10.0.0 to be exact) upstream and hides the fact that no tests completed. Thank you for the PR though.

@mscdex mscdex closed this Apr 30, 2018
@DanielRuf
Copy link
Author

To be honest, it is unclear to my why this should be a bug in Node 10 (is there some issue about this?) and the assert in there makes not much sense to me. I feel like this is intended and deprecated / due to internal changes which should work like this.

@mscdex
Copy link
Owner

mscdex commented Apr 30, 2018

The issue is that an arguably semver-major change was made to node only a few days before v10.0.0 was released and that is causing the tests to fail as-is. The assert is necessary because all tests should complete when the process exits. If not all of them did, then something went wrong and it'd be nice to be able to know that.

@DanielRuf
Copy link
Author

DanielRuf commented Apr 30, 2018

The assert is necessary because all tests should complete when the process exits

Huh? No this check does not do this. It also outputs Only finished 0/5 tests at the moment. assert just compares the values in the condition (to be equal), just like normal assert.equals works in unit testing in general.

@mscdex
Copy link
Owner

mscdex commented Apr 30, 2018

No this check does not do this.

Trust me, it does. t is only incremented in next(), which is only called after each test finishes. So if when the process exits and t !== tests.length, then obviously something is very wrong.

Additionally, only calling makeMsg() on its own is essentially a no-op, all it does is a build and return a string.

It also outputs Only finished 0/5 tests at the moment

Yes, but only on node v10.0.0 and for the reason I previously mentioned.

@DanielRuf DanielRuf deleted the test/node-10-fix-unit-tests branch April 30, 2018 22:40
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.

2 participants