Repository navigation
Replies: 10 comments 8 replies
|
Hi @MMicieli, Thank you for this thorough and well-structured review. I fully agree with your findings, the overall goal of improving efficiency and non-regression, and your proposed 4-stage order of work. Aligning Codecov with pytest is a great catch. Also I did saw those issues raised by SonarQube but yes many of them are non-urgent and many are related to code complexity. But at some point it will be needed to address the true positives high priority ones. Regarding how we organize the work: let's avoid opening a tracking issue or multiple smaller linked issues. I prefer to keep all the discussion and coordination right here in this thread, and address the improvements directly through consecutive, iterative PRs following your 4 stages (referencing #1269 in each PR). You can include the supporting evidence and verification criteria directly in the PR descriptions. Please feel free to send me the sanitized details regarding the credential-handling concern separately, and go ahead with the first PR for Stage 1 whenever you are ready! Thanks again for this initiative |
|
Stage 1, item 1 (coverage collection) is merged in #1270 (
The CodeCov job now takes about 18 minutes, up from about 8, because it runs the whole pytest suite. I have not proposed any speed-up yet. Next I plan to work on Stage 1 test isolation and ineffective assertions, in separate small PRs that reference #1269. |
|
Stage 1 update: #1274 — Open-Meteo test isolation is ready for review. The test-only PR fixes stale HTTP mocks and cache-dependent tests, with isolated fixtures and stronger regression assertions. Validation: 9,483 tests passed locally, all applicable CI checks passed, and independent review found no blocking defects. Sourcery findings have been addressed/responded to. |
|
Stage 1 progress update: #1275 — Solcast test isolation: ready for review. Removes shared-cache dependencies and fragile cache rename/restore logic without changing production code. Next: remaining external-dependency and generated-file isolation, followed by a final assessment of outstanding Stage 1 correctness issues. |
|
Stage 1 update: #1276 — Forecast.Solar and list-cache test isolation is ready for review. This test-only PR fixes three cache-isolation defects, preventing tests from using or modifying shared repository cache files. It reuses the existing helper from #1275 and removes unnecessary cleanup code (+5/−12 lines). Validation: 9,483 tests passed locally, all 18 applicable CI checks passed (1 neutral), Sourcery approved, and independent review found no blocking issues. Next, I'll continue with the remaining Stage 1 reliability and correctness work. |
|
FYI - SonarCloud currently runs on master but is skipped for fork-based PRs, so new SonarCloud findings may not be identified until after merging. @davidusb-geek would you prefer to retain the existing advisory approach, investigate safe SonarCloud analysis for external PRs, or consider a new-code quality gate? Currently this is beyond current scope but something for you to consider (I am not to sure if it is worth doing or not but thought it was worth mentioning The existing #1269 work will continue independently regardless. |
|
Stage 1 update: #1281, is now open for review. #1281 contains five test-only corrections addressing unintended external HTTP requests, shared data/cache dependencies, generated repository files and fragile test behaviour. Validation: 9,483 tests passed locally, Ruff checks passed, zero external network attempts, and independent review approved. No production code changes. Once #1281 is resolved, I'll look at the remaining Stage 1 assertion-effectiveness findings before proceeding to Stage 2. When Are you planning another EMHASS version release? Do you want me to keep going or stop at stage 1 and do the rest in another release? |
|
The six focused PRs have now been merged: #1270, #1273, #1274, #1275, #1276 and #1281. These addressed CI test execution, ineffective assertions, external forecast-provider mock/cache isolation, and CLI/Home Assistant test side effects. The remaining confirmed Stage 1 findings have been consolidated into the final PR: #1282 — test: complete Stage 1 correctness and isolation fixes This covers the remaining ineffective HTTP and ML test assertions, repository-data writes during tests, and environment-variable contamination. From my side, the planned Stage 1 implementation and verification work is complete. I don't anticipate another Stage 1 PR unless tehre is an issue with #1282 Once you have released the next EMHASS version I will begin Stage 2 (solver and optimisation regression effectiveness) as a separate workstream. |
|
Release v0.18.8 is out, we should be able to continue with Stage 2 |
|
Stage 2 — Regression effectiveness Focus:
Note: A new vulnerability has been picked up (because of recent system improvements) that ill work on first and then move onto starting stage 2 properly. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Hi @davidusb-geek,
I've been reviewing the EMHASS test suite and CI workflows to see how effectively the tests detect regressions, where reliability could be improved, and whether some of the existing complexity could be reduced without sacrificing meaningful test coverage.
The review identified several areas worth addressing. Before opening individual issues or making changes, I wanted to share the findings and get your thoughts on the overall direction.
My aim isn't to introduce more testing infrastructure or restructure things unnecessarily. It's to make the existing tests more reliable, more effective at identifying actual problems, easier to maintain and, where possible, faster to execute.
1. Scope of the review
The review was conducted against
masterat commit179cbc2b, using the code and CI results available on 28–29 October 2026.It included:
The suite currently collects 9,588 pytest cases, including a substantial DST regression suite.
The controlled experiments used isolated copies on Windows/Python 3.12, with external network requests blocked and synthetic credentials. These were targeted investigations rather than a completed cross-platform QA run.
No changes were made to the upstream repository.
2. What's already working well
The review confirmed that several parts of the existing test suite provide valuable protection.
The DST regression tests are particularly useful. Controlled changes involving timestamp alignment, duplicated or missing time steps and daylight-saving transitions demonstrated that some apparently repetitive tests detect faults that others miss.
Several tests initially considered suitable for consolidation also proved to provide distinct protection.
For this reason, I wouldn't recommend reducing test counts simply for the sake of reducing them.
Any simplification should preserve the underlying scenarios and demonstrate that important regression detection remains intact.
3. Summary of findings
These are my suggested priorities for discussion, rather than proposed GitHub labels or an agreed implementation order.
4. Detailed findings
A. Test isolation and external dependencies
What I found
A small number of tests rely on external HTTP requests instead of fully controlled responses.
Some tests also depend on environment variables, previously populated caches or files generated by other tests.
One Open-Meteo test contains a mock whose URL no longer matches the application's request. It fails when executed independently with an empty cache, despite passing in a larger test run.
Other tests write generated optimisation files into the repository's data directory.
These behaviours introduce unnecessary network dependencies, test-order sensitivity and potential interference between concurrent test runs.
Suggested improvement
Make the affected tests self-contained by mocking external requests, isolating relevant environment variables and using temporary directories for generated data.
The intended result would be tests that pass independently and offline, without relying on previous execution or external services.
There is also a credential-handling concern associated with some test requests. I'll provide sanitised details separately rather than include potentially sensitive information here.
B. Tests that pass without checking the intended behaviour
What I found
Several tests appear to provide less regression protection than their names suggest.
For example, seven assertions pass generator expressions directly into
assertTrue. Because a generator object is truthy, these assertions pass without evaluating the conditions inside it.Other examples include:
Controlled fault injection confirmed that some tests continue passing when the relevant behaviour is deliberately changed.
Suggested improvement
Correct these tests so they explicitly check the behaviour described by their names.
Where practical, verify each correction by demonstrating that the test fails when the corresponding behaviour is deliberately broken.
This would strengthen the suite without requiring a large number of new tests.
C. Incomplete Codecov and SonarCloud coverage reporting
What I found
The main GitHub Actions workflow runs the pytest suite, with 9,588 collected cases.
However,
.github/workflows/codecov.yamlcurrently usesunittestfor coverage collection.The corresponding CI run reported 1,350 tests, meaning the coverage workflow does not measure the same test collection as the main workflow.
I reproduced the existing coverage measurement locally at approximately 85.0% Python line coverage.
Adding the previously omitted pytest-only modules and DST tests increased measured coverage to approximately 88.6%, representing 419 additional covered Python source lines.
These are local comparative measurements, not a completed replacement CI run, but they demonstrate a material difference in what is measured.
SonarCloud also reports a lower overall coverage percentage because it includes JavaScript files not covered by the Python coverage report.
Suggested improvement
Update the existing coverage workflow to use pytest rather than unittest, so Codecov and SonarCloud receive coverage information from the actual regression test suite.
Branch coverage could also be considered.
I don't think this requires another coverage tool or an entirely new CI workflow.
D. Solver-dependent optimisation tests
What I found
Several optimisation regression tests fail when the HiGHS random seed changes, even though the application code remains unchanged.
Some failures involve exact expected schedules where multiple solutions have the same objective value.
A solver can legitimately choose a different optimal schedule without necessarily introducing a regression.
One temperature-related case also changes under alternative seeds and needs separate investigation to determine whether it reflects numerical tolerance or a genuine constraint issue.
This is particularly relevant to CVXPY and HiGHS upgrades, which can change solver decisions.
Suggested improvement
Where multiple solutions are valid, consider checking the objective value, relevant constraints and physical invariants instead of requiring one exact schedule.
Exact schedule assertions should remain where a scenario genuinely requires a unique result.
The goal is to distinguish actual optimisation regressions from harmless differences in solver decisions.
E. Gaps in physical and numerical regression checks
What I found
The fault-injection experiments identified several specific areas where existing tests could provide stronger protection.
Examples include:
There is also an opportunity to use shared assertions for power balance, SOC bounds, efficiency relationships and other physical constraints across existing optimisation scenarios.
Suggested improvement
Introduce a small number of targeted checks and reusable physical-invariant assertions.
Rather than creating numerous new scenarios, these could be applied to existing tests where the required results are already available.
The verification would be whether they detect the relevant faults while continuing to accept valid alternative optimisation solutions.
F. Dependency compatibility and reproducibility
What I found
The existing CI matrix covers several Python versions and operating systems, which is useful.
However, dependency resolution generally favours newer versions within the declared ranges. Some lower supported versions are not explicitly tested.
A few tests also use unseeded random inputs or depend on the current clock.
These can make failures difficult to reproduce or hide compatibility problems until dependencies change.
Suggested improvement
Use deterministic test inputs where appropriate and consider limited, scheduled compatibility checks for important dependency versions.
I'd avoid expanding the full PR matrix unnecessarily.
Existing work, including #1220 and PR #1261, should guide the approach.
G. Test duplication, complexity and execution time
What I found
Some parts of the test suite, particularly
test_optimization.py, contain repeated setup, near-identical test functions and substantial duplicated scenario construction.Several groups appear suitable for parametrisation or shared test builders.
There are also opportunities to reduce execution time by removing external-request timeouts, avoiding unnecessary repeated work and making inputs deterministic.
However, the review demonstrated that apparent duplication doesn't always mean redundant regression protection.
Suggested improvement
Simplify incrementally, with each change preserving the existing scenarios and assertions or replacing them with stronger checks.
Where several functions are consolidated into one parametrised test, the individual executed cases should remain identifiable.
I wouldn't recommend a wholesale rewrite or splitting
test_optimization.pyas part of this initial effort.I'd also avoid adding new testing frameworks unless a specific requirement demonstrates that existing tools are insufficient.
5. SonarCloud findings
I also reviewed the existing SonarCloud results against the audit.
The quality gate is currently failing, although I haven't conclusively established which individual new-code findings are responsible.
Some reported issues overlap with the test problems identified above. Others concern UI code, complexity or coding conventions rather than demonstrated defects.
There are also findings that appear to be false positives or low-risk warnings.
I wouldn't recommend a blanket effort to clear every SonarCloud warning just to improve the metrics.
It would be more useful to address verified problems first and investigate other findings individually where there is a demonstrated benefit.
6. Proposed order of work
I'd suggest addressing this in four stages.
Stage 1 — Reliability and correctness
Address external requests, shared state, ineffective assertions, stale mocks and incomplete coverage collection.
Stage 2 — Regression effectiveness
Strengthen the tests that currently miss demonstrated faults, and improve optimisation assertions that depend unnecessarily on particular solver results.
Stage 3 — Compatibility and reproducibility
Review deterministic test inputs, important dependency versions, remaining edge cases and opportunities to reduce avoidable execution time.
Stage 4 — Simplification
Once the regression requirements are established, consolidate repeated setup and parametrise suitable tests.
Each change should be independently verifiable, preserve meaningful protection and avoid unrelated production modifications.
7. Existing issues and PRs
Some findings overlap with work already being discussed or developed.
Relevant examples include:
I'd work within those existing discussions wherever possible rather than duplicate their scope.
8. How I'd suggest organising the work
Rather than opening numerous issues immediately, I thought it would be better to discuss the findings and proposed approach here first.
If the direction makes sense, we could create one tracking issue covering the agreed work, then add smaller linked issues as individual improvements are ready.
Each issue would include the specific problem, supporting evidence, proposed correction and measurable acceptance criteria.
Changes could then be implemented through small PRs rather than one large test-suite overhaul.
This would also help distinguish confirmed defects from optional improvements and areas requiring further investigation.
9. Your thoughts, David?
I'd appreciate your thoughts before taking this further.
If you're comfortable with the approach, I'm happy to work through the agreed areas incrementally, preparing focused issues with supporting evidence and verification criteria before making changes.
I have the detailed audit findings, test outputs, controlled fault-injection results and coverage comparisons available, so the evidence can be supplied as needed without overwhelming this initial Discussion.
The goal is to strengthen confidence in the existing test suite, preserve the protection it already provides, reduce unnecessary complexity and make future EMHASS changes easier to validate.
Thanks, David.
All reactions