fix(test): Fix tests to pass on windows without disrupting other platforms

[zfbx]

Summary

Replaced dev/null path with a universal temp_dir function that persists through the test session and cleans itself up afterwards to function identically but runs on windows as well as others? I don't have another platform to test this on so it will need to be tested.

_{Sorry, it was bugging me I wanted tests to pass xD}

Tasks Completed

• Platforms Tested:
  • Windows x86
  • Windows ARM
  • macOS x86
  • macOS ARM
  • Linux x86
  • Linux ARM
• Tested For:
  • Basic functionality
  • PyInstaller executable

[Computerdores]

Haven't been able to test it yet, but it seems to be good apart from what I commented on.

[Computerdores]

Is there any reason you are not using tempfile.NamedTemporaryDirectory here? Also, if you change to that, it might make more sense to create a new one in each test in order to isolate the different tests from each other (I would in that case specifically suggest making use of it's ability to be used with with)

[zfbx]

I can't find any NamedTemporaryDirectory, only NamedTemporaryFile. I actually preferred when it would clean up after itself rather than leaving directories behind but I'm not sure what's proper in this sort of case. I've not had much experience in testing, I just wanted to get them to pass on windows 😅 I'm open to changes suggested if you think it would be better

[Computerdores]

My bad the class is just called TemporaryDirectory and, I think, you can access its Path via the property name, see https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory

[CyanVoxel]

This module creates temporary files and directories. It works on all supported platforms. TemporaryFile, NamedTemporaryFile, TemporaryDirectory, and SpooledTemporaryFile are high-level interfaces which provide automatic cleanup and can be used as context managers. mkstemp() and mkdtemp() are lower-level functions which require manual cleanup.

TemporaryDirectory seems to be an easy solution that cleans up after itself, and I agree that it might make more sense to use this in each relevant test rather than pass the same value around

[zfbx]

Updated to use TemporaryDirectory and tested working.

[CyanVoxel]

I've got a few more suggestions to look into after the most recent changes, some affecting pre-existing code that was seemingly trying to do what you're currently trying to do now.

I also caught an old note of mine inside the CacheManager that described special behavior that was in direct response to the "dev/null" assumptions in these tests up until now. This note and check can now be removed (cache_manager.py):

- # NOTE: The /dev/null check is a workaround for current test assumptions.
- if self.last_lib_path and self.last_lib_path != Path("/dev/null"):
+ if self.last_lib_path and self.last_lib_path:

[CyanVoxel]

So due to the high number of casts back and forth between str and Path for this temp directory I went and looked into what this whole hasattr block is actually doing. Turns out it's also using TemporaryDirectory but in a much more confusing and roundabout way. I've been able to greatly simplify this section of the code by removing it and using the temp_dir parameter directly - here I renamed it to library_dir:

@pytest.fixture
def library(library_dir: Path):
    thumbs_path = library_dir / ".TagStudio" / "thumbs"
    thumbs_path.mkdir(parents=True, exist_ok=True)

    lib = Library()
    status = lib.open_library(Path(library_dir), ":memory:")
    assert status.success

The single test that used this was test_refresh_new_files inside test_refresh_dir.py, which would now need the (simplified) parameters:

@pytest.mark.parametrize("exclude_mode", [True, False])
def test_refresh_new_files(library: Library, exclude_mode):

Finally if you did also decide to rename temp_dir to library_dir as I did, it would also need to be changed inside test_file_path_options.py. Honestly though it's a minor thing and I'd just like to see this fix be pulled before it stirs up more skeletons 😅

[zfbx]

I will see what I can do here 😅

[zfbx]

looked into what this whole hasattr block is actually doing. Turns out it's also using TemporaryDirectory but in a much more confusing and roundabout way. I've been able to greatly simplify this section of the code by removing it and using the temp_dir parameter directly - here I renamed it to library_dir:

Okay, I see what's going on, doing this was because of this on test_refresh_dir:test_refresh_new_files() needed a unique directory for it's test and it goes through library() to do so.

New changes up and pass on Windows x86 and Mac x86 (technically x64?)

[CyanVoxel]

I'm not sure what the difference between the default scope vs the session scope here in practice, as they both seem to generate different temp directories when this fixture is called.

The only tangible effect I can observe is having (scope="session") causes the tests in test_refresh_new_files() to fail if using my suggested removal of the strange pre-existing "hasattr" code...

[zfbx]

For this my understanding of it is that when you use the scope="session" fixture it runs the code inside it once and then any further calls to it return the same temporary directory path so it can be reused across multiple tests and then when the session is complete it runs cleanup and deletes the directory which is why the yield is used instead of return because return causes the program to end before TempDir does it's cleanup leaving the files in the temp directory.

Which leads to the other comment, Yeah, I did notice you had more uses of TemporaryDirectory in other test but I wasn't sure if there was a reason for it maybe needing fresh directories for different tests. I only built it this way because it seemed test_file_path_options:test_title_update() depended on the path being exactly the same as used with conftest:library() which is probably why you had it the way you did.

I hope any of this made sense cause I'm only like 80% following my own words here myself

[Computerdores]

The changes look good!
What I noticed though is that the json migration test fails for me and I can't properly check why because I keep getting this error (this was already present before the tests broke on windows):

d:\Coding\TagStudio\tests\test_json_migration.py::test_json_migration failed with error:  
 The python test process was terminated before it could exit on its own, the process errored with: Code: 3221226505, Signal: null

And the only way to circumvent that is to run all tests at once, really weird.
Only thing I can tell atm is that check_field_parity is what fails

[Computerdores]

The changes look good! What I noticed though is that the json migration test fails for me and I can't properly check why because I keep getting this error (this was already present before the tests broke on windows):

d:\Coding\TagStudio\tests\test_json_migration.py::test_json_migration failed with error:  
 The python test process was terminated before it could exit on its own, the process errored with: Code: 3221226505, Signal: null

And the only way to circumvent that is to run all tests at once, really weird. Only thing I can tell atm is that check_field_parity is what fails

Ok I also got this error:

And running the tests twice in the terminal had the migration tests fail in only one of the runs... I think this might just be weirdness of my setup.

[CyanVoxel]

Sorry for the delay on getting back to this - Everything seems to function on my end (macOS + Windows) and it looks like things are ready to pull.
Thank you for your work on this!