Fix tests for node@16 error format change

[Nargonath]

Closes #4282.

[Nargonath]

Forgot I had to keep the old message format for node < 16 😅

[devinivy]

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 process.versions.v8. V8 doesn't follow semver, but the first two numbers reflect the chromium milestone divided by 10 (e.g. chromium M93 <-> v8 9.3), so a comparison like this should work:

// 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

[Nargonath]

Yes you're right @devinivy. I'll make the test more precise.

FWIW I prefer to test against the node version than v8. It makes it more easily understandable to others IMO.

[kanongil]

This seems a bit over-engineered. I would just change the test to:

expect(event.error.message).to.include('Cannot read prop');

[Nargonath]

@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.

[kanongil]

I know my suggestion is not perfect, but I still think it is a better solution due to the simplicity.

[cjihrig]

The only problem I see with testing that way is that it could shadow a similar error for another field

You could add a second assertion for the field name.

[Nargonath]

You could add a second assertion for the field name.

Yes I was just about to say it, you beat me to it.

I know my suggestion is not perfect, but I still think it is a better solution due to the simplicity.

I like your suggestion @kanongil, I'll do it that way and assert on the field name also.

[kanongil]

👍