This PR fixes a bug in how impression data is reported when impressionDataAll is set to true: When a feature has impressionData: false but impressionDataAll is set to true, it will now report impressionData: false instead of impressionData: undefined.
Why
This is a bug because we should report the state of impression data for a feature when we can. In the original feature PR discussion, it was agreed to make impressionData a boolean | undefined where it would be reported as a boolean if we knew the state and undefined otherwise.
If you don't have the try/catch and one of the expectations fail, then the done callback is never called (because the function short circuits). While this does mean that the test fails (this is good), it fails with a message saying that it exceeded the set timeout and not because it failed an expectation (this is bad).
To instead have the correct error message reported, you must catch the error and pass it to the done callback. This will make the test fail and report the correct error message.
Now regarding using finally, I did consider that but ended up not doing it for the following reason:
Calling the done callback requires the error for it to fail. This can't be done in a finally block because you don't have access to the error. In theory, you could probably declare an error variable that you set to undefined and then assign in the catch block, but that doesn't feel any cleaner to me.
What
This PR fixes a bug in how impression data is reported when
impressionDataAllis set to true: When a feature hasimpressionData: falsebutimpressionDataAllis set to true, it will now reportimpressionData: falseinstead ofimpressionData: undefined.Why
This is a bug because we should report the state of impression data for a feature when we can. In the original feature PR discussion, it was agreed to make
impressionDataaboolean | undefinedwhere it would be reported as a boolean if we knew the state andundefinedotherwise.It seems to have been an oversight in the tests.
Clarifications
What's with the try/catch blocks in the tests?
It's explained more thoroughly in the callback section of Jest's testing asynchronous code, but in short:
If you don't have the try/catch and one of the expectations fail, then the done callback is never called (because the function short circuits). While this does mean that the test fails (this is good), it fails with a message saying that it exceeded the set timeout and not because it failed an expectation (this is bad).
To instead have the correct error message reported, you must catch the error and pass it to the done callback. This will make the test fail and report the correct error message.
Now regarding using
finally, I did consider that but ended up not doing it for the following reason:Calling the done callback requires the error for it to fail. This can't be done in a finally block because you don't have access to the error. In theory, you could probably declare an error variable that you set to undefined and then assign in the catch block, but that doesn't feel any cleaner to me.