endlessm / kolibri-explore-plugin

The kolibri plugin to add the custom channel representation
MIT License
1 stars 4 forks source link

Content downloader tests #897

Closed dbnicholson closed 10 months ago

dbnicholson commented 10 months ago

:warning: Another giant PR!

This is big, but the vast majority is test data and scaffolding for running the tasks worker. If you're interested in getting into the minutiae of navigating Kolibri, Django and pytest-django, that's great. However, the interesting part is the last commit. The first commit has an internal interface change, and the second commit changes an internal interface return value. The rest is all test stuff including a vast pile of test data.

I'd really like to land this so I can tackle #890 with at least some confidence.

Fixes: #778

dbnicholson commented 10 months ago

The downloader test was a bit flaky locally. After running it repeatedly with extensive logging, it seems that Kolibri's task worker gets hung occasionally. I couldn't figure out why that would happen, so I added pytest-retry to retry the test when it times out in either the foreground or background downloads. The 3rd commit attempts to handle one potential issue I thought of while debugging.

dbnicholson commented 10 months ago

This is looking good to me, and it works on my system, albeit with some sporadic failures which I'm gathering are more related to problems we need to fix in the code being tested. I do have one suggestion with regards to making this a little less huge, but all seems good otherwise :)

I'm still getting the sporadic failures, too. I thought pytest-retry would fix that, but it actually doesn't do anything useful because it causes an error in the teardown of the failed test. I really wanted to get to the bottom of why Kolibri's worker stalls.

dbnicholson commented 10 months ago

This is impressive! Should we revert the retry as you guys have found that is not preventing the sporadic failures? From my side this is good to go and very welcome, even with those failures.

I think so, but let me play with it for a little bit longer. I was using pytest-flaky, but I think I should use pytest-rerunfailures, which has almost the same interface but is maintained by the pytest developers. I think that's more likely to work correctly.

dbnicholson commented 10 months ago

This is impressive! Should we revert the retry as you guys have found that is not preventing the sporadic failures? From my side this is good to go and very welcome, even with those failures.

I think so, but let me play with it for a little bit longer. I was using pytest-flaky, but I think I should use pytest-rerunfailures, which has almost the same interface but is maintained by the pytest developers. I think that's more likely to work correctly.

Yeah, pytest-rerunfailures seems to work correctly. Let me update the PR.

dbnicholson commented 10 months ago

This is impressive! Should we revert the retry as you guys have found that is not preventing the sporadic failures? From my side this is good to go and very welcome, even with those failures.

I think so, but let me play with it for a little bit longer. I was using pytest-flaky, but I think I should use pytest-rerunfailures, which has almost the same interface but is maintained by the pytest developers. I think that's more likely to work correctly.

Yeah, pytest-rerunfailures seems to work correctly. Let me update the PR.

If you're running this locally (@dylanmccall), you should pip uninstall pytest-retry since it provides the same flaky mark and I don't know how pytest will decide which one to use.

dbnicholson commented 10 months ago

Great, the rerun worked this time:

kolibri_explore_plugin/test/test_collectionviews.py::test_download_manager_clean[athlete-0001] RERUN [ 63%]
kolibri_explore_plugin/test/test_collectionviews.py::test_download_manager_clean[athlete-0001] PASSED [ 63%]

I'm going to merge this now.