Updated the test for extract_zipped_paths to run correctly if test_utils.py is changed

[ShivamBang98]

Hey! So I was playing around with requests and pytest and trying to see how unit tests work. While messing around with the tests cases in test_utils.py, I ran into a weird error, a completely unrelated unit test began to fail!

This test turned out to be the test for the the function extract_zipped_paths in utils.py. It seems that this function tries to extract a zipped file to a specific location, but doesn't do so if a file already exists at that location.

In the unit test for this function, the test file itself is being used to validate whether the file is properly extracted post zipping. However, after running the test suite once, the zipped file is never replaced again! This led to newer versions of this file being compared with an older version, which ultimately caused the test case to fail in the filecmp assertion.

Instead of me having to rm -rf it every time, I decided to include a minor cleanup step in the test case itself. Hope this helps out any future devs!

Note: I'm open to other suggestions as well. I tried to keep the underlying code/functionality as unchanged as possible, another approach could be to make a separate file just to test out this function? Could be a randomly generated file during tests, or a fixed one in the main repo as well.

[sigmavirus24]

The tmpdir fixture used by this test generates a random test directory every time it runs and cleans it up after the test is finished. I'm not sure how you're encountering the failure you describe and there's certainly not enough detail here to help debug this further.

[ShivamBang98]

I'll add a bit more context. I'm aware that the fixture creates a new temp directory, my assertion here is that the issue is actually in the behaviour of the extract_zipped_paths function, and that the function itself was creating a file outside of the temp dir. I'll try my best to explain it with screenshots/console logs below:

1. So, when I first ran this test suite, it ran fine. I then changed around some of the other tests, but this one began to fail with this error:

AssertionError: assert False
E        +  where False = <function cmp at 0x105cbd3a0>('/var/folders/w7/s_wdvyhn17947ttg6354fs5w0000gp/T/test_utils.py', '/Users/shivambang/practice/requests/tests/test_utils.py')
E        +    where <function cmp at 0x105cbd3a0> = filecmp.cmp

tests/test_utils.py:337: AssertionError

2. The directory /var/folders/w7/s_wdvyhn17947ttg6354fs5w0000gp/T/ turned out to contain my pytest-of-shivambang sub-directory, which is the correct expected behaviour. However, surprisingly, it also contained a file called test_utils.py.

3. It is my understanding that this file was created by the extract_zipped_paths function during the test execution. Since it's already been created once, it wasn't being overwritten on successive test runs. This ultimately caused any change to the test_utils.py file to fail this particular test case. Manually deleting the file via rm -rf fixed the problem, but it had to be done every time a change was made to this file.

Hope this explains it better, happy to share more details!

[ShivamBang98]

Steps to reproduce this error:

1. Run the test suite with pytest tests/test_utils.py
2. Make some edits to the test_utils.py file (any edit is fine, even a comment addition)
3. Re-run the test suite.

@sigmavirus24

[sigmavirus24]

I believe the correct fix here is to switch to tmp_path https://docs.pytest.org/en/stable/how-to/tmp_path.html#the-tmpdir-and-tmpdir-factory-fixtures since tmpdir is deprecated

[ShivamBang98]

Gotcha, I'll give it a shot. tmpdir is being used in a few different tests as well, I'll update all of those too and let you know.

[ShivamBang98]

Hey @sigmavirus24 , I've updated the tests in this file that were using tmpdir to now use tmp_path. However, my original fix containing os.remove is still needed, since the extract_zipped_paths function is extracting files out of the pytest tmp_path and directly into the system's temp directory. This can be seen in my attached screenshot, where tempfile.gettempdir() is being used to get the ultimate destination of the extracted file: