Skip to content

670 improve testing - #749

Merged
FlorianK13 merged 25 commits into
developfrom
670-improve-testing
Jun 9, 2026
Merged

670 improve testing#749
FlorianK13 merged 25 commits into
developfrom
670-improve-testing

Conversation

@FlorianK13

@FlorianK13 FlorianK13 commented May 22, 2026

Copy link
Copy Markdown
Member

General

This is a first step for making our tests better. It creates a mockup for the bulk download and it skips the API tests if no credentials are provided (which hopefully makes the CI run successfully, also for PRs from outside)

SUmmary of changes

  • Add tests/data/Gesamtdatenexport_mockup.zip: a small (~120 KB) mockup of the MaStR bulk download, generated by scripts/create_bulk_download_mockup.py. Contains 100 rows per XML table for wind, biomass, hydro

  • Refactor tests/conftest.py: replace the scan for existing local ZIP files with two fixtures (mockup_xml_zip_in_output_dir, mockup_docs_zip_in_output_dir) that copy the bundled files into a temporary directory. The output_dir fixture now sets MASTR_PROJECT_HOME_DIR via monkeypatch.setenv so the Mastr() instance automatically picks up the temp directory without needing an explicit output_dir argument.

  • Refactor tests/test_mastr.py — remove all @pytest.mark.skipif guards that depended on real data and split the single large download integration test smaller ones

  • Switch test_mastr_generate_data_model_fallback_to_included_docs from caplog to patch.object on the logger as somehow the caplog thing always failed

  • Refactor tests/soap_api/test_download.py — add a requires_real_credentials marker (using a runtime credential check) so that tests requiring real MaStR credentials are explicitly skipped when credentials are unavailable -> THIS HOPEFULLY FIXES THE GH CREDENTIAL ISSUE WHERE CI FAILED FOR OUTSIDE PRs

  • Delete tests/test_credentials.py — the credential test required real secrets and provided no value in CI.

  • Since pandas>3.0 it handles dtypes differently - i needed to adapt some tests so that mismatching dtypes do not cause failure anymore

PR-Assignee

Reviewer

  • 🐙 Follow the Reviewer Guidelines
  • 🐙 Provided feedback and show sufficient appreciation for the work done

@FlorianK13 FlorianK13 linked an issue May 22, 2026 that may be closed by this pull request
11 tasks
Basic refactoring to use mock bulk download
@FlorianK13 FlorianK13 self-assigned this May 22, 2026
@FlorianK13
FlorianK13 marked this pull request as ready for review May 22, 2026 15:40
These failed due to pandas 3.0 handles None types differently
@FlorianK13
FlorianK13 requested a review from Simon-Will May 22, 2026 15:59

@Simon-Will Simon-Will left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks a lot. It's looking quite good, I think! I only have some minor comments on most changes and one bigger thought:

I'm thinking a bit about how to trim the XML files and which files to choose for testing on. The current tests always work with wind, biomass, water and nuclear since these are small files and can be imported in a reasonable time for a test. I want to make two independent points:

  • With trimmed files, we can also in a short time import multiple files because they only contain 100 records each. This way, we would also test the multi-file imports, which is currently a blind spot in the tests.
  • The trimming works by using the first 100 records in a file. I think these are also the oldest records in the MaStR database, so they are quite a biased sample. It could happen that newer records contain some data that cannot be imported, but this would never be tested. This could be alleviated by choosing a random sample in the trimming process or by using solar so that we automatically have newer records by always having the first 100 records in each file.
    ** To be honest, this point is a bit moot since we already don't test the majority of the file types. If there were some data in some of them that couldn't be imported, the tests wouldn't catch it anyway.

What do you think about these?

Comment thread tests/soap_api/test_download.py
Comment thread tests/xml_download/test_utils_cleansing_bulk.py Outdated
Comment thread tests/test_mastr.py
@FlorianK13

FlorianK13 commented May 27, 2026

Copy link
Copy Markdown
Member Author

With trimmed files, we can also in a short time import multiple files because they only contain 100 records each. This way, we would also test the multi-file imports, which is currently a blind spot in the tests.

I am not 100% sure what you mean here. Is your suggestion to also include large tables like solar, storages, ...? If yes I think I agree. The script will take a lot longer to run, but we only need to run it once in a while. And it has the advantage that more (all) technologies are part of the tests.

Edit: Now I understand what you mean with multi-file: This is about technologies that have multiple xml files, like EinheitenSolar_1, _2, ... I will think of a way, maybe have maximum of three files per technology.

@FlorianK13

Copy link
Copy Markdown
Member Author

@Simon-Will can you re-review? I resolved your requested changes and did the following:

  • The bulk download mockup now includes all tables from the db.download() docs
  • If zipped downloads consist of one file, it will be written to one mockup file with 100 entries
  • If zipped downloads consist of two or more files, they will be written to exactly four mockup file with 25 entries. The algorithm puts roughly an equal amount of entries from each xml file into the final mockup file.
    • at the end only some entries are thrown away to make sure that each mockup has at most 100 entries
  • I added a few more data tests, but then stopped as rigorous testing would be part of the next PR

@FlorianK13
FlorianK13 requested a review from Simon-Will June 1, 2026 12:43
@Simon-Will

Copy link
Copy Markdown
Contributor

@FlorianK13, hi, I was on vacation last week, but now took a look. I'm a bit confused since I cannot see new commits, the > 0 assertion is still there and the mockup seems like it hasn't changed. Have you maybe forgotten to push the changes? Or am I missing something?

@FlorianK13

Copy link
Copy Markdown
Member Author

yeah sorry, forgot to push. Now the branch is ready for your review @Simon-Will

@Simon-Will Simon-Will left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the new creation of the mockup files. Very nice! Looks good to me!

@FlorianK13
FlorianK13 merged commit 430223d into develop Jun 9, 2026
9 checks passed
@FlorianK13
FlorianK13 deleted the 670-improve-testing branch June 9, 2026 06:15
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.

Streamline unit and integration tests

2 participants