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

bpo-28113: Convert Win32{Symlink,Junction}Tests to use support.TESTFN #12907

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

berkerpeksag
Copy link
Member

@berkerpeksag berkerpeksag commented Apr 22, 2019

Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

Note that Win32JunctionTests also uses os.path.dirname(os.path.abspath(__file__)); that should be amended as well.

dirlink = 'dirlinktest'
dirlink_target = os.path.dirname(filelink_target)
missing_link = 'missing link'
dirlink_target = os.path.join(support.TESTFN, 'win32_symlink_tests')
Copy link
Contributor

Choose a reason for hiding this comment

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

support.TESTFN is just a path, but not an existing directory.

Perhaps add something the following to setUp()?

        tempdir = tempfile.TemporaryDirectory(prefix=support.TESTFN)
        self.addCleanup(tempdir.cleanup)

        self.dirlink_target = os.path.join(tempdir , 'win32_symlink_tests')
        self.filelink_target = os.path.join(self.dirlink_target, support.TESTFN)
        self.dirlink = os.path.join(tempdir , 'dirlinktest')
        self.filelink = os.path.join(tempdir , 'filelinktest')
        self.missing_link = os.path.join(tempdir , 'missinglinktest')

With this all of the other existence checks and cleanups may be removed.

@@ -2200,16 +2199,12 @@ def test_remove_directory_link_to_missing_target(self):
# was created with target_is_dir==True.
os.remove(self.missing_link)

@unittest.skip("currently fails; consider for improvement")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this no longer skipped: has this behavior been implemented?

Likewise for the next test below.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants