fix: green the test suite, and the two real defects it was hiding - #28
Merged
Conversation
The suite has been red on main: 6 failures and 13 collection errors, with one file that could not even import. A red suite that everyone steps around stops being a signal, and it was hiding two genuine bugs. TWO REAL DEFECTS FIXED 1. Corruption recovery crashed on the corruption it was handling. _recover_from_corruption backs the bad file up, resets to defaults and calls _save(). _save() re-reads the file first, to preserve an api_key written externally by api_key_manager - and on that path the corrupt bytes are still on disk, so json.load raised a JSONDecodeError that _save did not catch (it caught only OSError). A user whose config.json got truncated got a crash instead of the automatic recovery the code was written to provide. The read now falls back to an empty merge and overwrites. 2. The main window lost its minimum size. _restore_window_geometry() applies whatever width and height were saved to config, and setMinimumSize had gone missing, so there was no floor - one bad saved value reopens the app as a sliver, on a window whose ticker table alone reserves 300px of height. A test was asserting 800x600; rather than delete the assertion to go green, the guarantee is restored. STALE TESTS BROUGHT TO THE CURRENT DESIGN - test_market_hours.py could not import: it wanted US_MARKET_HOLIDAYS, a static list replaced by get_market_holidays_for_year(). It now exercises the generator, including 2031 - a year outside any hardcoded table, which is the regression the computed version exists to prevent. - test_monitor.py and test_api_integration.py asserted that _check_ticker calls alert_manager.send_high_alert. It has returned a PendingAlert since alerts were consolidated; _send_consolidated_alerts does the notifying so several tickers crossing in one sweep become one notification. They also never set first_check_done, so price-gap protection suppressed the alert they were waiting for. Both now test the real contract, plus new coverage that the first observation never alerts and that the cooldown window opens again. - test_config.py required ConfigError on a corrupt file. Recovery is the deliberate behaviour; it now asserts recovery, that the bad file is preserved byte-for-byte alongside, and that valid JSON is left on disk - which is what pins defect 1. - test_finnhub.py asserted tokens_available strictly decreased. The limiter is a module-level singleton drained by earlier tests in the same file, and it refills against time.monotonic(), so the assertion raced the refill. It now resets the bucket and allows for refill. Freezing the clock instead DEADLOCKS: acquire(blocking=True) waits for a refill that cannot arrive and its own 30s timeout is measured on the same frozen clock. That is recorded in the test. - test_main_window.py read the ticker table by bare column index and a "Name" column had been inserted at 4, so four tests read the wrong cell while asserting confidently about the right one. Indices are now named constants in one place. Tab count 2 -> 5, and the title is app.description (app.name is the QApplication name, a different string). 98 passed, 0 failed, 0 errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The suite has been red on
main: 6 failures and 13 collection errors, with one file that could not even import. A red suite everyone steps around stops being a signal — and it was hiding two genuine bugs.Two real defects fixed
1. Corruption recovery crashed on the corruption it was handling.
_recover_from_corruptionbacks the bad file up, resets to defaults and calls_save()._save()re-reads the file first to preserve anapi_keywritten externally — and on that path the corrupt bytes are still on disk, sojson.loadraised aJSONDecodeErrorthat_savedid not catch (it caught onlyOSError). A user whoseconfig.jsongot truncated got a crash instead of the automatic recovery the code was written to provide.2. The main window lost its minimum size.
_restore_window_geometry()applies whatever width and height were saved to config, andsetMinimumSizehad gone missing — so there is no floor, and one bad saved value reopens the app as a sliver, on a window whose ticker table alone reserves 300px of height. A test was asserting 800x600; rather than delete the assertion to go green, the guarantee is restored.Stale tests brought to the current design
test_market_hours.pycould not import — it wantedUS_MARKET_HOLIDAYS, a static list replaced byget_market_holidays_for_year(). Now exercises the generator, including 2031 — a year outside any hardcoded table, which is the regression the computed version exists to prevent.test_monitor.py/test_api_integration.pyasserted_check_tickercallsalert_manager.send_high_alert. It has returned aPendingAlertsince alerts were consolidated. They also never setfirst_check_done, so price-gap protection suppressed the alert they were waiting for. Now test the real contract, plus new coverage that the first observation never alerts and that the cooldown window reopens.test_config.pyrequiredConfigErroron a corrupt file. Recovery is the deliberate behaviour; it now asserts recovery and that the bad file is preserved byte-for-byte — which is what pins defect 1.test_finnhub.pyassertedtokens_availablestrictly decreased. The limiter is a module-level singleton drained by earlier tests, and it refills againsttime.monotonic(), so the assertion raced the refill. Freezing the clock instead deadlocks —acquire(blocking=True)waits for a refill that cannot arrive and its own 30s timeout runs on the same frozen clock. Recorded in the test.test_main_window.pyread the ticker table by bare column index, and a "Name" column had been inserted at 4 — so four tests read the wrong cell while asserting confidently about the right one. Indices are now named constants in one place. Tab count 2 → 5; title isapp.description(app.nameis the QApplication name, a different string).98 passed, 0 failed, 0 errors.
🤖 Generated with Claude Code