Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed failed test, due to error message not matching. #6106

Closed
wants to merge 1 commit into from
Closed

Fixed failed test, due to error message not matching. #6106

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/app/__tests__/UpdateServerManager.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,12 @@ describe('UpdateServerManager', function () {

it('should throw error when URL is invalid', async () => {
let testee = fixture.createTestee();
let invalidURL = 'foobar';
try {
await testee.getUpdateArchive({link: 'foobar'});
await testee.getUpdateArchive({link: invalidURL});
assert.fail('Expected error not thrown!');
} catch(error) {
assert.equal(error.message, 'Invalid URL: foobar');
assert.equal(error.message + ':' + invalidURL, 'Invalid URL:'+ invalidURL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@omar908 omar908 Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ronny1982!

The error is coming from node.js internals, not the repo of the application.
image

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!

Copy link
Contributor

@ronny1982 ronny1982 Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ronny1982 ronny1982 Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

I suggest to leave the test unchanged, but properly adjust the build pipeline to use NodeJS 12

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, they were 12.x previously. Those changes within the github actions workflow were made here, I THINK:
73826d5

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
});

Expand Down
Loading