Fixed failed test, due to error message not matching. #6106
Conversation
| assert.fail('Expected error not thrown!'); | ||
| } catch(error) { | ||
| assert.equal(error.message, 'Invalid URL: foobar'); | ||
| assert.equal(error.message + ':' + invalidURL, 'Invalid URL:'+ invalidURL); |
There was a problem hiding this comment.
This assertion does not look correct to me.
The error message seems to not contain the URL anymore.
The old assertion looks correct to me, so the production code probably needs to be fixed and not the test.
There was a problem hiding this comment.
Hi @ronny1982!
The error is coming from node.js internals, not the repo of the application.
I just finished double checking on this to be safe.
Changes were made on the Node.js internals to remove the invalidURL from the error message, please see below of the change that was made:
nodejs/node@417c31b
Thank you for the feedback!
There was a problem hiding this comment.
So this means the test output depends on the NodeJS version on which the test is performed?
In this case, the test should be adjusted to work with any NodeJS version and not just with the latest, or the test should be kept as is and the node version for this project needs to be locked to the same version that is shipped with the HakuNeko Desktop Client (Electron 8.3.4 => NodeJS 12.13.0).
May also affect the build pipeline
There was a problem hiding this comment.
OK, it seems the build agents are installing NodeJS 16.x instead of the required 10.x which explains why the test is breaking now on the pipeline.
I suggest to leave the test unchanged, but properly adjust the build pipeline to use NodeJS 12
There was a problem hiding this comment.
Correct, they were 12.x previously. Those changes within the github actions workflow were made here, I THINK:
73826d5
There was a problem hiding this comment.
I can adjust the test to work with either version, but first we need to decide, do we want to go back to 12.x on the github workflow as well, in that case I would double check to see if the issue even exists once we go back.
There was a problem hiding this comment.
Sorry, I didn't see the bottom of you message, my mistake.
Let me make a separate PR to change the build pipeline and double check the test then.
Fixed failed test, due to error message not matching.