Skip to content

feat(scenarios): add firmware update success scenario#133

Open
YANGCHUNHONG3000 wants to merge 2 commits into
ocpp-debugkit:mainfrom
YANGCHUNHONG3000:feat/firmware-update-success-scenario
Open

feat(scenarios): add firmware update success scenario#133
YANGCHUNHONG3000 wants to merge 2 commits into
ocpp-debugkit:mainfrom
YANGCHUNHONG3000:feat/firmware-update-success-scenario

Conversation

@YANGCHUNHONG3000

Copy link
Copy Markdown

Add a new scenario to verify that a station undergoing a successful firmware update does not trigger any false positives:

  • expectedFailures: [] — The trace describes a clean firmware update flow (BootNotification → FirmwareStatusNotification(Dowloaded) → FirmwareStatusNotification(Installed) → Heartbeat), which should not trigger any detection rule.
  • assertions: [{ type: 'no_failures', params: {} }] — Verifies that no failures are detected, acting as a regression guard against overly aggressive detection rules.
  • tests/external-fixture/test.mjs was also updated (line 161: 1516) because it has a hard-coded scenario count assertion.
  • All 520 tests pass (19 test files).

@sepehr-safari

Copy link
Copy Markdown
Member

Thanks for this, and for writing up the reasoning in the description. The scenario
does the job it was asked to do: FIRMWARE_UPDATE_FAILURE is a real rule in
detection.ts, and a clean Downloaded then Installed flow is a sensible negative
control against it firing where it should not. I ran the branch locally and lint,
typecheck, build, the 520 unit tests, and the external fixture test all pass.

Four things before I merge. The first two are on the PR, the last two are on me
for leaving them out of the issue.

  1. Prettier. pnpm format:check fails on
    packages/toolkit/src/scenarios/__scenarios__/firmware-update-success.ts.
    Three of the message arrays are wrapped at 80 columns and this repo is set to
    100. pnpm format fixes it. CI runs the format check right after lint, so it
    would have stopped there. That step is missing from the contributing guide's
    local verification list, which is my gap and I am fixing it separately.

  2. Changeset. The PR adds a new public export (firmwareUpdateSuccessScenario)
    and changes the length of the exported scenarios array, so it needs a
    changeset. pnpm changeset, pick a minor bump for @ocpp-debugkit/toolkit,
    and commit the generated file.

  3. Station ID. CS-SYNTHETIC-012 is already used by meter-anomaly.ts. The
    existing scenarios each use a distinct ID, currently 004 through 015, so this
    one wants CS-SYNTHETIC-016. The issue told you to use the CS-SYNTHETIC-NNN
    shape without saying they have to be unique, so this one is on the issue text.

  4. Counts in the docs. Adding a scenario also makes these two lines stale:
    README.md:25 and packages/toolkit/README.md:23, both of which say
    "15 predefined scenarios". Happy for you to bump them here since it is a
    one-word edit in each. CONTRIBUTING.md needs the same bump plus an unrelated
    correction, so leave that file alone and I will take it. The counts in
    CURRENT_STATE.md are historical milestone entries and should stay as they are.

I will approve the workflow run so you get CI output on the next push.

One note on scope, separate from this PR. You claimed #104, #106, #107 and #108
together. I would rather assign these one at a time, so #104 is yours and I will
hold the others open. Once this merges, tell me which one you want next and I
will assign it. That is a limit in how I worded the claim instructions rather
than anything you did wrong, and I am rewording them.

sepehr-safari added a commit that referenced this pull request Jul 26, 2026
…e state (#135)

PR #133, the first external contribution to a good-first-issue, failed CI and
needed four review items. Three of the four came from gaps in the contributor
guide rather than from anything the contributor did.

CONTRIBUTING.md now documents `pnpm format:check`, which CI enforces and the
guide never named, with both command lists ordered to match the CI job step for
step and a note that CI stops at the first failure. The hard-coded counts are out
of the architecture table: the detection entry claimed 15 rules and there have
been 16 since REPEATED_BOOT_NOTIFICATION landed in #114, and those counts have
now drifted twice. "Adding a Scenario" names all four files carrying the scenario
count and flags that only two of them fail the suite when missed. Station IDs are
documented as unique per scenario. Good-first-issues carry a claim policy of one
open claim at a time.

Also corrects the v0.4.x release state in CURRENT_STATE.md, which reported 0.4.1
with a 0.4.2 patch in progress. 0.4.2 is published and holds the latest dist-tag,
its tag and GitHub release exist, and both bugs it tracked (#127, #128) are
closed. The active milestone now reads v0.5.0 with v0.4.x complete, What's Done
gains the v0.4.x release record it never had, and What's Next drops the request
to cut v0.4.0, which shipped on 2026-07-17.

No changeset: neither file is in the package `files` list, so nothing here ships
to npm.

Closes #134
Closes #136
@YANGCHUNHONG3000

Copy link
Copy Markdown
Author

@sepehr-safari Thanks for the thorough review!

I've pushed a fix commit addressing items 2–4:

  • ✅ Station ID: CS-SYNTHETIC-012CS-SYNTHETIC-016
  • ✅ Changeset added (minor bump for @ocpp-debugkit/toolkit)
  • ✅ README counts bumped: "15 predefined scenarios" → "16"

For item 1 (Prettier formatting), I noticed the new scenario file doesn't seem to have message arrays wrapped at 80 columns in my local view — could you point me to the specific lines that need pnpm format? Happy to push another commit once I know exactly what needs fixing.

Also, thanks for clarifying the claim workflow — noted for next time. I'd like to take #104 next once this merges.

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.

2 participants