test: fix intrinsics test #26660
test: fix intrinsics test #26660
Conversation
So far this test did not verify that the call did indeed fail since the error case was not checked. This makes sure the error is indeed thrown as expected.
lundibundi
added
the
author ready
| } catch { | ||
| } | ||
| assert.throws( | ||
| () => Object.defineProperty = 'asdf', |
There was a problem hiding this comment.
Ignorable style micro-nit that others might not agree with anyway: I prefer braces ({ and }) in situations like this because without braces, the arrow function returns a value, and IMO we shouldn't return a value unless we are intending to return a value. Here, we are returning a value as a style side effect. Someone reading this might not know if the return value is significant. By adding braces, we make it clear it's not.
There are, of course arguments the other way. (Someone who knows how assert.throws() works will know the return value is irrelevant. Return a value or not, who cares really?) Just my opinion. I think...uh...@sam-github maybe? Someone else thinks the same way. But it's hardly universal on the project....
Anyway, code change looks good with or without this suggestion. But if I've managed to convince you, cool. 😆
There was a problem hiding this comment.
I agree with you, but since the assignment throws an error, there is in fact no return value 😁
So far this test did not verify that the call did indeed fail since the error case was not checked. This makes sure the error is indeed thrown as expected. PR-URL: nodejs#26660 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
|
Landed in 6d09012 |
So far this test did not verify that the call did indeed fail since the error case was not checked. This makes sure the error is indeed thrown as expected. PR-URL: nodejs#26660 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
So far this test did not verify that the call did indeed fail since the error case was not checked. This makes sure the error is indeed thrown as expected. PR-URL: #26660 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
8 participants
So far this test did not verify that the call did indeed fail since
the error case was not checked. This makes sure the error is indeed
thrown as expected.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes