fix: Unhandled promise rejection in tests #95
Conversation
| expect(err.message).to.equal("invalid payload"); | ||
| }) | ||
| .catch((err) => { | ||
| console.error(err); |
There was a problem hiding this comment.
I think this should fail the test. If the message is not correct, it will throw. This seems to indicate that there's something wrong with the test itself. It's failing but not registering as a failure.
There was a problem hiding this comment.
PR updated, please take a look when you have a time. 👍
|
@helio-frota have you take a look at @lance's requested changes? |
|
@fabiojose yeah I still need to understand the issue. I agree with @lance that probably the test has something wrong. But in other hand that test will be removed since it is related to spec 0.3 it("Throw error when the event does not follow the spec 0.3", () => {This PR is just to remove the Node warning: UnhandledPromiseRejectionWarning: AssertionError: expected 'failed' to equal 'invalid payload'When we deal with another catch block, we can see clearly the result: {
message: "expected 'failed' to equal 'invalid payload'",
showDiff: true,
actual: 'failed',
expected: 'invalid payload'
}I think it is better to close this PR and wait for that warning message to be removed without a discarded effort. 😃 Please, let me know what do you think 👍 |
|
@helio-frota I did some experimenting with this over the weekend. These tests are particularly problematic in a way that the 1.0 version of the tests is not, because the 0.3 unmarshalling is async. Any of the unmarshalling tests for 0.3 will exhibit this same behavior. When the call to I think we need to determine whether 0.3 is going to be removed. If not, we need to fix this for all of the 0.3 unmarshaling tests. If 0.3 goes away, then this problem magically disappears on |
|
Seems good now and able to be merged 👍 |
| .catch((err) => | ||
| expect(err.message).to.equal("invalid payload")); | ||
| try { | ||
| await un.unmarshall(payload, headers); |
There was a problem hiding this comment.
@helio-frota you should fail the test after this line. The expectation is that the unmarshalling should throw if the incoming message does not conform to spec. As the test is now, that isn't happening. We never enter the catch block below. You can change that .to.equal() call to be .to.equal('tacos') and the test still passes.
There was a problem hiding this comment.
OOps you right! I was only focused on solve the unhandled promise exception.
Thanks for the review, commit updated.
There was a problem hiding this comment.
It still not good btw..
I'm trying to understand what is happening...
Signed-off-by: Helio Frota <00hf11@gmail.com>
|
Ok I found that If the |
|
@helio-frota as we keep nibbling around the edges of what is a bigger problem, I'm starting to think that the right move here is to modify the 0.3 unmarshaling API so that it's no longer asynchronous and matches the API exposed for the 1.0 protocol. There is no reason that this API should be async. Maybe the best plan of attack is to open a new PR that addresses that. WDYT? |
|
@lance I agree with that. (Not saying that I'm going to send a PR soon afterwards 😄 ) But anyway related to this specific issue I think we are good, the |
This commit removes the unnecessary use of Promises in the 0.3 unmarshaller. There was actually no asynchronous activity happening in that function, so there was no need to deal with Promises, and as a result testing was made much more difficult. Fixes: cloudevents#95 Signed-off-by: Lance Ball <lball@redhat.com>
|
Closing - superseded by #126 |
This commit removes the unnecessary use of Promises in the 0.3 unmarshaller. There was actually no asynchronous activity happening in that function, so there was no need to deal with Promises, and as a result testing was made much more difficult. Fixes: #95 Signed-off-by: Lance Ball <lball@redhat.com>
No description provided.