You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While reviewing PR #7248 (added tests/test_borlange_energi_se.py alongside a
parser fix), I checked two things that turned out to be connected. Posting
both together since one is the cause of the other.
1. pytest.ini silently excludes all per-source test files from CI
python_files is an allowlist of exactly two filenames. Verified with pytest tests/ --collect-only --ignore=tests/test_source_components.py:
only the 4 tests in test_fetch_retry.py are collected. None of the
following ever run in CI's bare pytest invocation (validate.yaml):
Consequence: at least 4 of these already fail when run in isolation
(icon-mapping drift, a renamed exception import, a stale mock target), and
nothing ever caught it, because CI has never actually executed them. They've
been silently decorative since whenever they were added.
2. Should we even want a tests/test_<source>.py file per source by default?
Separately from the CI-wiring question: with ~600 sources, one dedicated
test file per source doesn't scale well and risks becoming unmanageable
clutter in tests/. TEST_CASES + a live run of test_sources.py -s <name> -l is already the standard coverage path for a
source, and is what reviewers actually check today.
Proposal: keep dedicated per-source unit test files as something a
contributor may add for genuinely non-trivial logic (a tricky date/regex
parser, say — the Borlänge one is actually a good example of this done
well: pure function, mocks nothing because it needs nothing, no network),
but stop treating it as a default expectation, and say so explicitly in the
AI-assistant instruction files so agents don't start generating one for
every new source PR.
Don't add a dedicated tests/test_<source>.py unit-test file by default. TEST_CASES plus a live run of test_sources.py -s <name> -l is the
normal coverage for a source. Only add a standalone unit test when a
source has genuinely non-trivial logic worth isolating that TEST_CASES
can't exercise well on its own, and mock any network calls in it rather
than hitting the live API.
Decisions needed
CI wiring: widen python_files to actually collect these (after
fixing the 4 currently-broken ones), or leave the allowlist as the
deliberate CI scope and just document that per-source test files aren't
automatically run?
Policy: adopt the "don't add by default" wording above in the
AI-assistant instruction files, yes/no/amend?
Not proposing a PR yet — flagging for discussion first since it touches
project-wide contributor guidance.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
Uh oh!
There was an error while loading. Please reload this page.
While reviewing PR #7248 (added
tests/test_borlange_energi_se.pyalongside aparser fix), I checked two things that turned out to be connected. Posting
both together since one is the cause of the other.
1.
pytest.inisilently excludes all per-source test files from CIRoot
pytest.ini:python_filesis an allowlist of exactly two filenames. Verified withpytest tests/ --collect-only --ignore=tests/test_source_components.py:only the 4 tests in
test_fetch_retry.pyare collected. None of thefollowing ever run in CI's bare
pytestinvocation (validate.yaml):tests/test_colchester_gov_uk.pytests/test_darebin_vic_gov_au.pytests/test_heinz_entsorgung_de.pytests/test_hillingdon_gov_uk.pytests/test_midlothian_gov_uk.pytests/test_olo_sk.pytests/test_southlanarkshire_gov_uk.pytests/test_stockport_gov_uk.pytests/test_sudestavenir_fr.pytests/test_borlange_energi_se.py(new, from fix(borlange_energi_se): handle "Tömning idag" / "imorgon" #7248)Consequence: at least 4 of these already fail when run in isolation
(icon-mapping drift, a renamed exception import, a stale mock target), and
nothing ever caught it, because CI has never actually executed them. They've
been silently decorative since whenever they were added.
2. Should we even want a
tests/test_<source>.pyfile per source by default?Separately from the CI-wiring question: with ~600 sources, one dedicated
test file per source doesn't scale well and risks becoming unmanageable
clutter in
tests/.TEST_CASES+ a live run oftest_sources.py -s <name> -lis already the standard coverage path for asource, and is what reviewers actually check today.
Proposal: keep dedicated per-source unit test files as something a
contributor may add for genuinely non-trivial logic (a tricky date/regex
parser, say — the Borlänge one is actually a good example of this done
well: pure function, mocks nothing because it needs nothing, no network),
but stop treating it as a default expectation, and say so explicitly in the
AI-assistant instruction files so agents don't start generating one for
every new source PR.
Draft wording (not yet committed anywhere) for
AGENTS.md/CLAUDE.md/.github/copilot-instructions.md:Decisions needed
python_filesto actually collect these (afterfixing the 4 currently-broken ones), or leave the allowlist as the
deliberate CI scope and just document that per-source test files aren't
automatically run?
AI-assistant instruction files, yes/no/amend?
Not proposing a PR yet — flagging for discussion first since it touches
project-wide contributor guidance.
All reactions