-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 tests for node@16 error format change #4283
Conversation
Forgot I had to keep the old message format for node < 16 😅 |
9966591
to
e4a1970
Compare
This update to V8 occurred in node v16.9.0. Could/should we check for node >= v16.9.0 rather than >= v16, even if just to help keep this precisely documented? Another option is to base the check on the version of V8 // e.g. '9.0.257.24-node.11' -> ['9', '0'] -> '90' -> 90
const toChromiumMilestone = (ver) => parseInt(ver.split('.').slice(0, 2).join(''), 10);
toChromiumMilestone(process.versions.v8) >= 93 |
Yes you're right @devinivy. I'll make the test more precise. FWIW I prefer to test against the node version than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit over-engineered. I would just change the test to:
expect(event.error.message).to.include('Cannot read prop');
@kanongil The only problem I see with testing that way is that it could shadow a similar error for another field which might not be the problem we want to test here. It's probably unlikely though. |
I know my suggestion is not perfect, but I still think it is a better solution due to the simplicity. |
You could add a second assertion for the field name. |
Yes I was just about to say it, you beat me to it.
I like your suggestion @kanongil, I'll do it that way and assert on the field name also. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Closes #4282.