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

Content downloader tests #897

Merged
merged 15 commits into from
Oct 27, 2023
Merged

Content downloader tests #897

merged 15 commits into from
Oct 27, 2023

Conversation

dbnicholson
Copy link
Member

⚠️ 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

As a convenience, the generateed `remotecontentimport` task was setting
`node_ids` to an empty list if it wasn't specified. However, an empty
list and `None` are 2 different cases. An empty list is used when no
node IDs are to be imported, and `None` is used when all node IDs are to
be imported. Even though we're not actually downloading all nodes here,
fix the semantics and require that the thumbnails tasks explicitly
request no nodes. This will be used later in testing.
This will be used in testing to check that a task was actually enqueued.
The storage hook depends on the `BackgroundTask` containing the current
job ID. However, it's possible the job will change state and the hook
will run before the job ID has been saved. That would prevent the hook
from properly synchronizing the updated job state. To prevent that, lock
the database while the task is being enqueued until the job ID is saved.
By default set the log level to INFO so there's useful information on
failures. Task handling is multithreaded, so add the thread name to help
debugging.
The logs won't be shown if the tests pass, but if they don't the
messages can be invaluable for debugging failures.
Learning Equality obviously has no use for the test suite constantly
telling it about an ephemeral deployment. More importantly, the ping
task can hang and interfere with our own tasks being scheduled.
In order to exercise the collection downloader, we need channel data.
The `create_channeldb_json.py` script and `db.json.template` file are
used to create JSON files representing fake channels. The generated JSON
files contain the data necessary to create sqlite channel databases with
sqlalchemy as well as the content data inlined. 18 fake channels have
been created and will be used in later commits.
The `create_contentdir` function takes the fake channel DB JSON files
and creates a content directory with the channel databases and content
files for testing. That's made available as the `contentdir` pytest
fixture.

A standalone script is provided as a convenience and for exercising the
functionality outside of the test suite.
This runs an HTTP server for the test content so that we can import
channels and content during tests just like they were being imported
from studio.
Provide test collections using all the packs in the current
endless-key-collections release. The fake packs have very regular
structures and use the fake channel data.
These exercise all of the API endpoints that don't interact with the
download manager.
This adds a few pytest fixtures for running channel and content import
jobs. Beyond creating a facility and a facility user with appropriate
permissions, 2 fixtures for handling interactions between Django and
SQLAlchemy. Some of this is lifted from Kolibri and/or depends on
Kolibri internals. We'll see how well they hold up over time.
While here, add a few log messages to aid when debugging failures.
@dbnicholson
Copy link
Member Author

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.

Copy link
Collaborator

@dylanmccall dylanmccall left a comment

Choose a reason for hiding this comment

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

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 :)

kolibri_explore_plugin/test/plugin.py Show resolved Hide resolved
@dbnicholson
Copy link
Member Author

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.

Copy link
Collaborator

@manuq manuq left a comment

Choose a reason for hiding this comment

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

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.

@dbnicholson
Copy link
Member Author

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
Copy link
Member Author

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.

It seems that Kolibri's task worker can hang occasionally, so use
pytest-rerunfailures to mark tests that can be retried on failure.
This is a basic smoketest that the collection downloader can be run with
any collection specified. Unfortunately, Kolibri's task worker seems to
hang sometimes during either the foreground or background downloading.
Use pytest-rerunfailures's `flaky` mark to try again when that happens.

Fixes: #778
@dbnicholson
Copy link
Member Author

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
Copy link
Member Author

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.

@dbnicholson dbnicholson merged commit 2a12060 into master Oct 27, 2023
3 checks passed
@dbnicholson dbnicholson deleted the 778-downloader-tests branch October 27, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add collection downloader tests
3 participants