assert: fix diff color output #19464
Conversation
nodejs-github-bot
added
the
errors
The color output was broken at some point and that was not detected because it was not tested for properly so far. This makes sure the colors work again and it adds a regression test as well.
|
Rebased due to conflicts. New CI https://ci.nodejs.org/job/node-test-pull-request/13856/ |
The color output was broken at some point and that was not detected because it was not tested for properly so far. This makes sure the colors work again and it adds a regression test as well. PR-URL: nodejs#19464 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
Landed in 2e6dd93 |
|
Should this be backported to |
The color output was broken at some point and that was not detected because it was not tested for properly so far. This makes sure the colors work again and it adds a regression test as well. PR-URL: nodejs#19464 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
|
Should those color values be calculated just once instead of on every AssertionError instantiation? The comment When else would those env vars change after the first AssertionError? |
|
@jasonkarns it is theoretically possible to change environment settings at any point of time. As far as I know there is no rule so far about when to check for environment settings. There are multiple possibilities:
In this case I decided to go for the latter without having a strong opinion on either case. Do you see a downside in this specific behavior? |
|
@BridgeAR yep, theoretically possible since of course anything under process. is mutable :D Just seems that practically speaking, these assertions will be used primarily for test suites, and it would be extremely unlikely for the system under test to modify those values in between tests. The only downside is just the "waste" of recalculating those values when, in practice, they'll never change. Almost more valuable is the code cleanliness of just having them defined once as I'm typically on team "don't pre-optimize". But in this case it just feels weird to have the code contort and be defensive about such a scenario. 🤷♂️ |
|
@jasonkarns the overhead for this is pretty tiny compared to creating the stack trace etc. I checked our code base again and in most cases we check the env on each use. Since there is no rule to it so far we should likely keep this behavior in place (what ever reason a user might have to change the environment variables during runtime). |
The color output was broken at some point and that was not detected
because it was not tested for properly so far. This makes sure the
colors work again and it adds a regression test as well.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes