Need some advice on a couple of things:

• For some of the dataset testing, updating the cache dir call and checking for python version was enough for the tearDown to work as is
• However, for the task testing, there are get_task calls hidden away inside of different unit tests which dump files and I believe creates lock directories.
  • Instead of creating checks for specific IDs, I felt it would be easier to scale if we have an os.walk collecting generated files and deleting them during tearDown
  • Kindly advise if this is not acceptable or maybe why this wasn't a part of the design previously.
  • If we look up for files in the test/files/ directory by checkpointing files before and after the unit tests, it appears to be a safe way of removing files generated during only that unit test
• If this design is acceptable, we can then maybe consider a class level setup and teardown to call the file list collection and deletion at the end of a Test Case class instead of each individual unit test (for some gain in execution time).
• Or we can do the same scan-collect-cleanup using a session level fixture which I believe would mean we now call every unit test (at least the ones which create files) with an argument passing the fixture.

Ideally, I was thinking it might be easier to scale across developers if the clean up happens at the TestBase superclass level so that every unit test need not be modified for this purpose.

My first concern on test-level clean-up is accidentally deleting files which were not generated by the executed unit test, but by a different unit test that is being executed in parallel (by default for CI we run up to 4 tests in parallel). Unless you know for a fact which files were generated by the unit test, I would not delete temporary files while other tests are still running. Per session clean up should avoid this issue, if I understand it correctly.

I wasn't personally involved with the initial unit test design, so I can't tell if not doing os.walk is a design choice. My best bet is that initially clean-up worked, somehow along the way it didn't anymore, but as everyone's busy and there were more important issues, it just did not get resolved. Again, that's just a guess. Perhaps @janvanrijn knows.

Could you point me to a specific task unit test that leaves behind files?

@PGijsbers, these tests (from the develop branch) should dump files.

test__get_task
test_get_train_and_test_split_indices

Both of those tests seem to work with the static cache directory that is purposefully part of the repositories. I don't see new files being generated for this. I'm sorry if I missed the point, but which files are generated/downloaded and left behind, but should not be?

I just checked on the develop branch, on running pytest -s tests/test_tasks/test_task_functions.py -k 'test__get_task', I get the following untracked files on git status:

• tests/files/org/openml/test/datasets/2/dataset.pkl.py3
• tests/files/org/openml/test/locks/
• tests/files/org/openml/test/tasks/1882/datasplits.pkl.py3

Is this reproducible?

generated/downloaded and left behind, but should not be?

Given that these 3 paths/files are generated by running this test case, I presume these are to be cleared, as issue #447 mentions. That's my assumption and hence this approach.

You're right, my bad. I can reproduce this. The files should definitely be cleaned up, at least after all unit tests are complete.

The problem with cleaning up these files after each unit test, is that we tend to use the same tasks/datasets across multiple unit tests to speed them up (make use of caching). This violates unit test independence, but I think is worth the benefit.
Cleaning up the lock directory at any other level than session defeats (part of) its purpose.

Any files that would only be used/generated by one unit test should be cleaned up immediately. But I don't think that's the case for any of those three files.

Alright. I'll then try to see how a session-level fixture works, and also maybe a class level tearDown. Though the latter again may not allow the leveraging of caching.
If at all os.walk is permissible, it is best to have a start and end checkpoint for the entire unit test. Shall explore the design possibilities.

Codecov Report

Merging #721 into develop will decrease coverage by 0.09%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           develop     #721     +/-   ##
==========================================
- Coverage    87.78%   87.68%   -0.1%     
==========================================
  Files           36       36             
  Lines         3961     3979     +18     
==========================================
+ Hits          3477     3489     +12     
- Misses         484      490      +6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b660d7d...1a6044c. Read the comment docs.

@PGijsbers
The primary reason I am not collecting files from each unit test to delete later is to prevent the cleanup onus from being with the developer writing test cases. In order to avoid that, I have gone ahead with os.walk.

However, to abate your valid concerns, the session-level fixture with yield seemed to work well with os.walk. At this point of time, any new test module that is written needs to only import the TestBase class along with the fixture_cleanup function. The yield defers the cleanup until the pytest session runs all the collected unit tests. This removes the problem of deleting the cache or other files (haven't tested, but assuming that parallel processes spawned are also part of the parent session) needed by different unit tests.

There are two things that I would like your opinion on:

1. I am partially hard-coding the file path here. From what I figured, to enable the singular call per module per session, this fixture function needs to be outside of a class definition (or gets called repeatedly). Therefore, I couldn't create static_cache_dir as the setUp of TestBase does.

2. Since the fixture_cleanup function needs to be imported but is not explicitly used, PEP8 throws the imported but unused issue. However, this seems to be a known issue with PyTest fixtures. One way to remove this should be to define the function in each of the test_xxx_yyy.py files before the class definitions. What do you think is a better trade-off here?

1. What about using __file__?
2. I am probably missing something from the pytest documentation. Could you elaborate on why the import of cleanup_fixture is necessary? Anyway, I assume it is, and if so, you can just silence Flake8 with adding # noqa: F401 (I think that's the corresponding error code).

Could you elaborate on why the import of cleanup_fixture is necessary?

From what I understand, the session-level fixture decorator allows a function to be executed ahead of all the unit tests collected. The unit test collection happens based on the directory/module/unit_test format I believe. If TestBase was defined in the same directory and was part of the collection, this would have worked without importing or if we define this cleanup_fixture function with the fixture decorator in all the test_xxx_yyy.py in the tests/ directory. However, to avoid redundant definitions, I thought it would be neater to place it in openml/testing.py only once. Now when PyTest collects the unit tests in the test/ directory, it sees no decorated function to inject. Hence, the added import.

What about using __file__?

I tried this, however, to not be hard-coded at all, I still need the n_levels information (currently part of the design as default argument in TestBase's setUp()) to figure out how many levels to go back. As I understand, the tests/files directory is something that by design is expected to be there. Since TestBase's setUp() also uses n_levels to go back certain levels and check for files/. In that case, the current design which I thought of as hard-coded is not really a problem I feel. In fact, it looks like an implicit design assumption.

I think the primary design decision here would be the placement of the cleanup_fixture function.
Should there be just one copy and another PEP8 exception be added to ci_scripts/flake8_diff.sh or shall I put the fixture function in each test module defined?

@PGijsbers I am also looking to fix #261. I feel the solution will be quite close to the one for this PR. Shall I include it here or make a new PR?

@PGijsbers I put the fixture function into the TestBase class. Now the same behaviour is maintained without additional imports. Hence, removed the F401 PEP8 ignore too.

I think the primary design decision here would be the placement of the cleanup_fixture function.

Defining it once and importing it in other files does seem like the way to go to me.
Seems like you find a good way to do that now.

For future reference, if you had had to go with the ignore on F401 to achieve this, you can do this on a per-line basis:

from openml.testing import TestBase, cleanup_fixture  # noqa: F401

Ignores the warning for that specific line only. This would be better because otherwise other lines/files with actual unused imports might have been ignored too.

I am also looking to fix #261. I feel the solution will be quite close to the one for this PR. Shall I include it here or make a new PR?

I think a new PR is preferred.

Thanks for the tip. 😄

I think in that case, I am done with this PR unless there is anything more specific required.
I'll wait for the merge with develop and then create a PR for #261.

status=`git status --porcelain -b`
if [ "$status" != "## master..origin/master" ]; then
    exit 1
fi

Made a slight change to this design, to not have a branch-specific check.
Also, I used [[ instead of [ based on this.

Hope both the above is okay and if not I'll revert them.