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

fix TestFromDirSymlink on Windows due to missing drive-letter #283

Merged

Conversation

thaJeztah
Copy link
Contributor

@thaJeztah thaJeztah commented Aug 26, 2024

This test was failing, because it expected the drive-letter to always
be returned, which wasn't the case;

=== FAIL: fs TestFromDirSymlink (0.01s)
    ops_test.go:55: assertion failed: directory C:\Users\circleci\AppData\Local\Temp\test-from-dir-2260318643 does not match expected:
        \a\b\3
          target: expected C:\some\inexistent\link got \some\inexistent\link

This patch removes the special-case from d44953d,
which unconditionally added the drive-letter. Instead, we only change
the check to use the expected directory-separator.

@thaJeztah thaJeztah force-pushed the fix_TestFromDirSymlink_windows branch from 2533182 to b848e70 Compare August 26, 2024 10:09
@thaJeztah
Copy link
Contributor Author

thaJeztah commented Aug 26, 2024

Hm... nope; that didn't help. Looks like something else needs to be updated; perhaps as part of the fs.Equal itself?

=== Failed
=== FAIL: fs TestFromDirSymlink (0.01s)
    ops_test.go:59: assertion failed: directory C:\Users\circleci\AppData\Local\Temp\test-from-dir-2043988436 does not match expected:
        \a\b\3
          target: expected C:\some\inexistent\link got \some\inexistent\link

Hm.. looking again, it looks like it's actual failing on the part related to

if runtime.GOOS == "windows" {
link3 = filepath.Join(filepath.VolumeName(currentdir), link3)
}

Looks like that was added in;

Maybe that part is no longer needed; let me try just removing it

@thaJeztah thaJeztah force-pushed the fix_TestFromDirSymlink_windows branch from 2fe67a9 to 0b8a80e Compare August 26, 2024 11:17

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This test was failing, because it expected the drive-letter to always
be returned, which wasn't the case;

    === FAIL: fs TestFromDirSymlink (0.01s)
        ops_test.go:55: assertion failed: directory C:\Users\circleci\AppData\Local\Temp\test-from-dir-2260318643 does not match expected:
            \a\b\3
              target: expected C:\some\inexistent\link got \some\inexistent\link

This patch removes the special-case from d44953d,
which unconditionally added the drive-letter. Instead, we only change
the check to use the expected directory-separator.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

test

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah changed the title fix TestFromDirSymlink on Windows for tempdir without drive-letter fix TestFromDirSymlink on Windows due to missing drive-letter Aug 26, 2024
@thaJeztah thaJeztah force-pushed the fix_TestFromDirSymlink_windows branch from 0b8a80e to 109d43e Compare August 26, 2024 11:28
@thaJeztah thaJeztah marked this pull request as ready for review August 26, 2024 11:28
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you!

I guess the Go stdlib changed in a recent version? This test was passing for a long time.

@dnephin dnephin merged commit 65a3150 into gotestyourself:main Aug 29, 2024
6 checks passed
@thaJeztah thaJeztah deleted the fix_TestFromDirSymlink_windows branch August 29, 2024 06:39
@thaJeztah
Copy link
Contributor Author

Yeah, no clue either; also honestly don't have a Windows machine to test locally 😂. Given that the original fix was a workaround for CI, and nobody complained that the code itself wasn't working, I'm pretty sure nothing was severely broken, so just the test.

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.

None yet

2 participants