[SE-3989] - Fixes for Blockstore integration tests.

[Kelketek]

Description

This pull request repairs the integration tests for Blockstore/Libraries v2 by changing updating relevant code to be compatible with the latest version of edx-search.

It's not known how long these tests have been broken because they are not run by Jenkins. The breakage appears to have been caused in: openedx/edx-search#104

CC: @Golub-Sergey @bradenmacdonald

Jira Ticket

https://openedx.atlassian.net/browse/OSPR-5578

Testing instructions

Set up Blockstore, and run make testserver to launch the testserver. Then, in devstack run make studio-shell and run these commands, one at a time so you can easily see if any failed:

EDXAPP_TEST_ELASTICSEARCH_HOST=edx.devstack.elasticsearch7 EDXAPP_ENABLE_ELASTICSEARCH_FOR_TESTS=0 EDXAPP_RUN_BLOCKSTORE_TESTS=1 python -Wd -m pytest --ds=cms.envs.test openedx/core/lib/blockstore_api/ openedx/core/djangolib/tests/test_blockstore_cache.py openedx/core/djangoapps/content_libraries/tests/
EDXAPP_TEST_ELASTICSEARCH_HOST=edx.devstack.elasticsearch7 EDXAPP_ENABLE_ELASTICSEARCH_FOR_TESTS=0 EDXAPP_RUN_BLOCKSTORE_TESTS=1 python -Wd -m pytest --ds=lms.envs.test openedx/core/lib/blockstore_api/ openedx/core/djangolib/tests/test_blockstore_cache.py openedx/core/djangoapps/content_libraries/tests/
EDXAPP_TEST_ELASTICSEARCH_HOST=edx.devstack.elasticsearch7 EDXAPP_ENABLE_ELASTICSEARCH_FOR_TESTS=1 EDXAPP_RUN_BLOCKSTORE_TESTS=1 python -Wd -m pytest --ds=cms.envs.test openedx/core/lib/blockstore_api/ openedx/core/djangolib/tests/test_blockstore_cache.py openedx/core/djangoapps/content_libraries/tests/
EDXAPP_TEST_ELASTICSEARCH_HOST=edx.devstack.elasticsearch7 EDXAPP_ENABLE_ELASTICSEARCH_FOR_TESTS=1 EDXAPP_RUN_BLOCKSTORE_TESTS=1 python -Wd -m pytest --ds=lms.envs.test openedx/core/lib/blockstore_api/ openedx/core/djangolib/tests/test_blockstore_cache.py openedx/core/djangoapps/content_libraries/tests/

Note that the above are two lines more than the Blockstore integration test instructions mention. I'm not sure if it should be in the documentation there, but two codepaths exist in testing, and I needed to toggle using real elasticsearch to ensure they were both followed.

Deadline

I'm not sure-- it's unclear to me if this has broken anything in production, but I have to assume it has. Sooner is better.

Other information

The changes which broke the integration test removed the need for specifying the document type to every operation on the search index. This seems to have assumed that a feature of edx-search, allowing multiple document types, was never used, and so could be eliminated.

Unfortunately, the library indexes DID use this feature, and because the integration tests aren't run automatically, this escaped the notice of the devs who made this change. This means to make the tests pass, and search to work as expected, I've had to create an additional index.

I don't know the full implications of having done so. I can only imagine it will mean a time-consuming reindex for someone down the line when this is merged. I also don't know if it will mean a lot of duplicated data between the indexes. Any further information and context is appreciated.

Reviewers

• @arbrandes
• @bradenmacdonald

[openedx-webhooks]

Thanks for the pull request, @Kelketek! I've created OSPR-5578 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[natabene]

@Kelketek Thank you for your contribution, please let us know once it is ready for our review.

[bradenmacdonald]

The changes which broke the integration test removed the need for specifying the document type to every operation on the search index. This seems to have assumed that a feature of edx-search, allowing multiple document types, was never used, and so could be eliminated.

Unfortunately, the library indexes DID use this feature, and because the integration tests aren't run automatically, this escaped the notice of the devs who made this change. This means to make the tests pass, and search to work as expected, I've had to create an additional index.

I don't know the full implications of having done so. I can only imagine it will mean a time-consuming reindex for someone down the line when this is merged. I also don't know if it will mean a lot of duplicated data between the indexes. Any further information and context is appreciated.

Hmm, looks like you'll need some input from @Golub-Sergey or @stvstnfrd / @dianakhuang who worked on openedx/edx-search#104 . Could one of you please give some advice here?

BTW most library v2 blocks on edx.org are used by LabXchange which has its own ElasticSearch index, so the index issue probably mostly affects people developing and testing the new libraries MFE.

[arbrandes]

@Kelketek, the third code path (i.e., the CMS+Elasticsearch one), and only this one, is giving me failures. They're all connection failures to ES, though. In the interest of saving a little time, did you face the same problem?

Edit: Found the problem, but not the solution. On recent devstacks, only the ES7 container is deployed by default. So one has to add the following to the line:

EDXAPP_TEST_ELASTICSEARCH_HOST=edx.devstack.elasticsearch7

But then, as probably expected, I'm getting "query unknown" errors.

[dianakhuang]

From what I understand, the reason why doctypes were removed was because ES7 doesn't support them in the same format any longer. I'm not 100% up on the technical details of that but @Golub-Sergey can explain better than I can.

[Golub-Sergey]

@bradenmacdonald As I understand your question, for now from the ES7 document types was deprecated and we should work with index per document type. ES documentation. We can split our doc_types into indices or squash them into an index and it depends on our needs.
Edx platform and edx-search ES requirements were transferred from the ES1.5 to the ES7, so code base which uses ES must be updated.

[Kelketek]

@arbrandes I thought that I'd run all four, but you were right, that one wasn't working.

In fact it was pretty involved how badly it was broken. There were three issues:

1. The syntax no longer fit the new version of ElasticSearch, as you pointed out.
2. Changing the syntax to match what appeared to be equivalent modern syntax returned different results, and so it took several iterations to finally find an equivalently performing syntax.
3. Multiple tests used the same names for Blockstore objects. This one was a doozy, because these tests assumedly worked at one point. Maybe they didn't but everyone else made similar mistakes to me when trying to run them and thought they did. It's also possible that the upgrade to ElasticSearch and the resulting query changes meant that results that would have been filtered out between tests no longer were.

In any case I appear to have gotten it to work as expected and it should be ready for you now.

@Golub-Sergey If the issue is that 'Doctype is no longer supported and each type needs its own index', then that's in alignment with what this PR does and I think that means we're good to go. The only issue might be that this deployment might require running a reindexing command.

It might not in practice, however, since LabXChange is the only deployment we know of which is seriously leveraging Libraries V2 and it's got its own indexer. That might mean that these indexes are used by no one and so should be broken by no one. After all, no one complained that the searches on libraries suddenly failed, and we would have expected them to if they did, right? @bradenmacdonald @arbrandes thoughts?

[bradenmacdonald]

@Kelketek

It might not in practice, however, since LabXChange is the only deployment we know of which is seriously leveraging Libraries V2 and it's got its own indexer. That might mean that these indexes are used by no one and so should be broken by no one. After all, no one complained that the searches on libraries suddenly failed, and we would have expected them to if they did, right? @bradenmacdonald @arbrandes thoughts?

Yes, that's my understanding. People are testing libraries / BD-14 on stage, but only LabXchange is using them in prod, and they have their own elasticsearch index.

[arbrandes]

@Kelketek, tests are working fine-and-dandy, no matter what order I run them in. Code looks good, too. Thanks!

👍

• I tested this as instructed
• I read through the code

[Kelketek]

Thanks, @arbrandes . This is ready for you, @bradenmacdonald

[natabene]

@bradenmacdonald Please review, thank you !

[bradenmacdonald]

👍 Thanks for figuring out those fixes, @Kelketek ! Please squash into a single commit with a clear commit message and any useful context/details from this PR, and I'll ask for it to be merged.

• I tested this: ran the tests
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: n/a

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[Kelketek]

@bradenmacdonald Done!

[openedx-webhooks]

@Kelketek 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[kdmccormick]

Thanks Fox!

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.