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.

Sign up for GitHub

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

Jump to bottom

Clean up aggregation tests #313

Closed
wants to merge 5 commits into from
Closed

Clean up aggregation tests #313

wants to merge 5 commits into from

Conversation

jamesfisher-geo
Copy link
Collaborator

Related Issue(s):

Description:

General cleanup of the aggregation test in an effort to resolve #290. I am not able to reproduce the error locally, but I have also noticed the issue when testing is run on github. My best guess is that it is an issue with async tests running in parallel. I made the following changes.

  • removed all unnecessary uses of the ctx fixture.
  • removed call for load_test_data

PR Checklist:

  • Code is formatted and linted (run pre-commit run --all-files)
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable
  • Changes are added to the changelog

@jamesfisher-geo
Copy link
Collaborator Author

And here it is 😅

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED stac_fastapi/tests/extensions/test_aggregation.py::test_post_aggregate_total_count - assert 2 == 1
=========== 1 failed, 189 passed, 18 skipped, 193 warnings in 18.94s ===========
Error: Process completed with exit code 1.

@jamesfisher-geo
Copy link
Collaborator Author

Found it! It is a simple bug I believe. For Elasticsearch, the indices() function was only checking if the input was None. But the test_post_aggregate_total_count test passed an empty list. So the assert 2 == 1 was happening because the aggregation was including the collection AND the item.

This has gotten a bit messy. I am going to close this PR and make the fix in a separate PR.

@jamesfisher-geo jamesfisher-geo deleted the aggregation_tests branch November 24, 2024 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test error occasionally
1 participant
@jamesfisher-geo