Skip to content

feat(scenarios): add meter-value-negative to the corpus - #189

Merged
sepehr-safari merged 2 commits into
ocpp-debugkit:mainfrom
wufangyong973:feat/meter-value-negative
Oct 11, 2026
Merged

sepehr-safari merged 2 commits into
ocpp-debugkit:mainfrom
wufangyong973:feat/meter-value-negative

Conversation

@wufangyong973

Copy link
Copy Markdown
Contributor

closes #182

METER_VALUE_ANOMALY has two branches and only the decreasing one was covered by the corpus, so a change that dropped the negative check would have sailed through the whole suite. This adds meter-value-negative: a cumulative register that reads -50 Wh, which physically can't happen, driving the negative branch directly.

Files: the scenario itself, registered in index.ts and asserted in index.test.ts (count, name order, getScenario, and an engine run that confirms the failure fires). The external fixture test and both READMEs bump 23 to 24. A patch changeset sits in .changeset since a new scenario does add a public export.

wufangyong973 and others added 2 commits October 2, 2026 11:47
METER_VALUE_ANOMALY has two branches and only the decreasing one was
exercised. This trace drives the register to -50 Wh, which a cumulative
register can never read, so the negative check now has a regression guard.
Registers in index.ts and index.test.ts, bumps the external fixture and
both README counts, and adds a patch changeset.
The BootNotification CallResult sat on one line over the 100 character
print width, so format:check failed before the suite ran.
@sepehr-safari
sepehr-safari merged commit ca5a318 into ocpp-debugkit:main Oct 11, 2026
2 checks passed
@sepehr-safari

Copy link
Copy Markdown
Member

Thank you for this, and sorry about the ten day wait, which was mine rather than anything to do with your push: GitHub holds the first workflow run from a new contributor until a maintainer releases it, and yours sat untouched showing you nothing. I have run the full check at your head commit and the only thing wrong was format:check, where the BootNotification response sat one line over the 100 character print width, so I have pushed that one line reformatted to your branch rather than make you wait on me a second time. Everything else was already right: the scenario reports METER_VALUE_ANOMALY and nothing else, and because there is a single reading the monotonicity comparison never has a pair to look at, so the negative check really is the only branch that can fire, which is the part that mattered. One correction, and it is to my issue rather than to your work: #182 says dropping the negative check would pass the whole suite, and your description repeats it, but that is not true and I should have checked. detection.test.ts has long had a detects negative meter value case, and deleting the negative block fails two tests, yours and that one. Yours is still worth merging for a narrower reason, which is that the unit test hands Event objects straight to detectFailures while yours is the only thing that carries a negative register from raw trace JSON through parseTrace and buildSessionTimeline, and that path is where this project's real bugs have been. I have fixed the issue body so it stops misleading people. Merged, and thank you again for a clean, well scoped change.

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.

Add a scenario: meter-value-negative

2 participants