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

Fix dataset tests #562

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix dataset tests #562

wants to merge 2 commits into from

Conversation

zain-sohail
Copy link
Member

@zain-sohail zain-sohail commented Feb 25, 2025

Fixes #559 and also moves the dataset documentation to an easily accessible place for users (also changed to md file for ease of updating).

Currently available here: https://opencompes.github.io/docs/sed/develop/user_guide/dataset.html (might change when another build is done)

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 13530332879

Details

  • 47 of 47 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 92.275%

Totals Coverage Status
Change from base Build 13498273640: 0.004%
Covered Lines: 7800
Relevant Lines: 8453

💛 - Coveralls

@zain-sohail zain-sohail requested a review from rettigl February 27, 2025 15:43
@rettigl
Copy link
Member

rettigl commented Feb 28, 2025

It fails on python versions 3.11, 3.12, with errors in creating temp directories. Not sure about the reason.

json_path_user = USER_CONFIG_PATH.joinpath("datasets.json")
def test_get_remove_dataset(requests_mock, zip_buffer, tmp_path):
"""Test to get a dataset, verify its directory and files, and then remove the dataset."""
json_path_user = tmp_path / USER_CONFIG_PATH / "datasets.json"
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't use a temp path, but the user config. Not sure why. I think you should patch the USER_CONFIG_PATH instead.

@rettigl
Copy link
Member

rettigl commented Feb 28, 2025

There are strong interconnections and race conditions between the different tests, as they all access the same datasets.json files as defined by the static dm. This needs also to be resolved, parallel running test very often fail randomly.

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.

Datasetmanager tests race condition
3 participants