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.

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

Add non-mocked test for symlink #1308

Merged
merged 10 commits into from
Mar 22, 2025
Merged

Add non-mocked test for symlink #1308

merged 10 commits into from
Mar 22, 2025

Conversation

coyotte508
Copy link
Member

@coyotte508 coyotte508 commented Mar 21, 2025

Alternative to #1307

Fix #1306

I was just too confused by the src/dst params, nodej's official docs could be clearer too.

Now the params are explictly named and there is a real test to check the filesystem contents.

By the way, it's a copy operation not a rename (because it was already the case, the new_blob param was never passed)

Also took the opportunty to use relative symlinks cc @Wauplin for better windows support

cc @jeffmaury @axel7083

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

I just a little bit concerned because the new tests don't pass on Windows which is why createSymlink has been created and as I'm on Windows it will make my life harder to check I'm not introducing regression

@coyotte508
Copy link
Member Author

I just a little bit concerned because the new tests don't pass on Windows which is why createSymlink has been created and as I'm on Windows it will make my life harder to check I'm not introducing regression

Ok, I added a mocked test on linux, where I just fail the symlink function to behave like it does on windows.

And in the other tests, instead of expect(stats.isSymbolicLink()).toBe(true);, I added expect(stats.isSymbolicLink()).toBe(process.platform !== "win32");

Hopefully the tests will now work for you on windows with those changes (and linux at the same time)

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM. But there are changes in XetBlob that seem unrelated

...(await importOriginal<typeof import("node:fs/promises")>()),
symlink: async (...args: any[]) => {
if (failSymlink) {
failSymlink = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure to get why we are changing the flag here

Copy link
Member Author

Choose a reason for hiding this comment

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

It's to reset the function once it's called. (so it doesn't impact other tests)

Could be done more elegantly

@coyotte508 coyotte508 merged commit bf4ee02 into main Mar 22, 2025
5 checks passed
@coyotte508 coyotte508 deleted the symlinks branch March 22, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

createSymlink is deleting the wrong file before perfoming operations
2 participants