#560: cover custom in the parity oracle; drop the query_file monkeypatch - #602
Merged
Conversation
…patch
`custom` needs no port. Its configuration arrives from the user's YAML
instrument file at runtime, and `declarations_from_configuration` reads a
ModelParameters instance as happily as a module - so the two-stage entry points
added in the previous PR already carry it end to end. Adding it as a parity case
is what keeps that true rather than accidental.
Getting it there exposed something in the registry. The oracle used to wrap
`query_file` so one `cellpy.get()` yielded both frames; for `custom` the capture
came back empty. The cause is not the test: the registry imports loader modules
under their **bare** name, so the class it instantiates (`custom.DataLoader`) is
a different class object from `cellpy.readers.instruments.custom.DataLoader`.
Patching the latter silently did nothing.
type(loader).__module__ -> "custom"
m.DataLoader.__module__ -> "cellpy.readers.instruments.custom"
type(loader) is m.DataLoader -> False
That has consequences past this test - isinstance checks against the package-path
class fail for registry-created loaders, and any class-level state exists twice.
Filed separately rather than fixed here, to keep this branch scoped.
The oracle no longer patches anything: `parse()` and `declarations()` exist now,
so it drives the vendor stage through the public entry points. That removes the
import-order sensitivity as well, and exercises what the switchover will use.
The same-run property the wrapping gave up is covered by
test_loader_two_stage.py, which checks parse() agrees with what loader() parses.
Two wrong guesses on the way to that diagnosis (patch list too narrow; then
import order), both fixed by inspecting the class identity instead of reasoning
about it.
Full suite under PYTHONUTF8=1: 1189 passed, 0 failed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Five of the eight shipped configuration-driven dialects have no vendor file in the repository, so the parity oracle could not reach them. "The exception list is empty" therefore said less than it sounded like: the covered dialects agree, not every dialect. The bugs this arc has actually found are all configuration-level - a wrong reset granularity, duration strings cast to null, a column mapped to the wrong native name, a unit template left unresolved. None of them needs real vendor data to reproduce; they need a file spelled the way the configuration says. So each fixture is generated from the configuration's own declarations: column names, separator, decimal mark, skiprows, state keys. Coverage goes from three of eight configuration-driven dialects to six: maccor_txt_three, neware_txt_zero and neware_txt_two now have value parity against the legacy path, all with zero mismatches. maccor_txt_three exercises European decimal commas, which nothing did before. Mutation-tested rather than assumed green: disabling the PER_STEP derivation makes neware_txt_zero fail with a 0.5 mAh diff on both capacity columns. What this does not do is exercise vendor quirks - the odd row, the interrupted run, the stray 0xFF byte. Those still need real files. This closes the systematic half of the gap, not the empirical half, and the module docstring says so. Two findings while building it, both filed separately rather than fixed here: - Files with fewer than roughly 150 data rows fail to load with a bare IndexError when the delimiter is auto-detected (20 and 100 rows fail; 150 and 300 load). A short vendor file is a normal thing - a truncated run, a quick export - and today it fails with an error naming neither the file nor the cause. It is also why these fixtures have to be 300 rows. - maccor_txt_zero and batmo_bdf_bdf remain uncovered: the former has empty formatters and no post-processors and looks like a template rather than a loadable dialect; the latter needs its own generator shape. Full suite under PYTHONUTF8=1: 1195 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 20, 2026
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
1 participant
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.
Follow-up to #601. Part of #560. Tests only — no production code changes.
customneeded no portIts configuration arrives from the user's YAML instrument file at runtime, and
declarations_from_configurationreads aModelParametersinstance as happily as a module — so the two-stage entry points from #601 already carry it end to end. Verified before writing anything; the only change is adding it as a parity case so that stays true rather than accidental.Four parity cases now (maccor model one, neware model one, maccor model two, custom), and the unported-post-processor exception list is still empty.
What it uncovered in the registry
The oracle used to wrap
query_fileso onecellpy.get()yielded both frames. Forcustomthe capture came back empty — not a test bug. The registry imports loader modules under their bare name, so the class it instantiates is a different object from the one at the package path:Two distinct classes from one source file. Consequences beyond this test:
isinstance/issubclassagainst the package-path class fail for registry-created loaders, class-level state exists twice, and monkeypatching the package-path class silently does nothing — which is exactly how this surfaced.Not fixed here, to keep the branch scoped. Worth its own issue.
The oracle got simpler
parse()anddeclarations()exist now, so the oracle drives the vendor stage through the public entry points instead of patching. That removes the import-order sensitivity and exercises what the switchover will actually use.The "both frames from one run" property that wrapping provided is covered by
tests/test_loader_two_stage.py::test_the_two_stages_agree_with_the_legacy_frame— that check is what stops this module from verifying a path nothing uses.Verification
Full suite under
PYTHONUTF8=1(the Linux-proxy mode that caught the encoding bug in #601): 1189 passed, 0 failed.Next on #560
pec_csvis the genuinely hard one left: it extendsBaseLoaderdirectly — noAutoLoader, noconfig_params, nonormal_headers_renaming_dict— and matches headers dynamically through_HEADER_ALIASES. There is nothing to derive declarations from, so it needs either a hand-written declaration set or a refactor expressing its aliases as a configuration. A design choice rather than a mechanical port.Then
arbin_res(ODBC, so CI can only check it against the committed fixture), tier 2, and the switchover.🤖 Generated with Claude Code