Skip to content

test: formalize real PLC acceptance testing - #892

Open
gijzelaerr wants to merge 1 commit into
masterfrom
feat/840-real-plc-acceptance
Open

test: formalize real PLC acceptance testing#892
gijzelaerr wants to merge 1 commit into
masterfrom
feat/840-real-plc-acceptance

Conversation

@gijzelaerr

Copy link
Copy Markdown
Owner

Progresses #840.

Adds a pytest-bdd-based real-PLC acceptance program with 26 versioned scenarios covering connection, canonical reads, multi-read, reconnect behavior, and safely restored scratch writes across legacy S7 and S7CommPlus.

Safety and evidence:

  • smoke-only default runner; writes/admin require separate explicit opt-ins
  • shared protocol-neutral helpers used by the existing diagnostic e2e suites
  • scratch bytes restored and read-back verified from fixture teardown
  • Gherkin terminal output, JUnit XML, and schema-versioned sanitized JSON
  • issue form, stable repository labels, saved searches, fixture setup, and volunteer runbook
  • real-PLC tests remain opt-in and outside hosted CI

Local verification:

  • uv run pre-commit run --all-files
  • uv run pytest -q --tb=short (2079 passed, 109 skipped)
  • uv build --offline --quiet
  • uv run sphinx-build -W -b html doc /tmp/python-snap7-doc-840

The final acceptance item—filing one legacy-S7 and one S7CommPlus real-hardware result—requires volunteer PLC runs after this infrastructure lands, so this PR intentionally does not close #840.

@gijzelaerr

Copy link
Copy Markdown
Owner Author

@russwing Could you run the cross-model real-PLC acceptance pass for the S7CommPlus work once its code review settles? Your S7-1200 and S7-1500 access makes that the useful compatibility pair. Suggested order: the response/authentication roots (#881, #882, #883, #887), then stacked integrity/renewal (#884, #888), followed by typed tags/subscriptions (#886, #891). Please use the result template in this PR and record the exact CPU, firmware, protection mode, and tested commit. #881 still has an unresolved S7-1511C item-read failure from the latest reporter retest, so a failure there would also be valuable evidence.

@russwing

Copy link
Copy Markdown
Contributor

Ran the harness today on both CPUs of the compatibility pair. One blocking defect in the S7CommPlus adapter, and a preparation gap of ours. Commit eb6d9e3.

The blocker: the S7CommPlus adapter never enables TLS

tests/real_plc/support.py::S7CommPlusAdapter.connect calls:

self.client.connect(self.config.host, self.config.port, self.config.rack, self.config.slot)

S7CommPlusClient.connect defaults use_tls: bool = False, and its own docstring says "required for V2". S7CommPlus V2 is mandatory-TLS — S7-1200 firmware V4.5+ and S7-1500 V2.x+ — so the adapter cannot reach any modern CPU. Every s7commplus scenario fails at connect with S7ConnectionError: Connection closed by peer, which is the RST an unencrypted V1 SetupSession earns from a V2 target.

Controlled proof — same CPU, same commit, same port, only use_tls differs:

S7-1200 1212C V4.7.3     use_tls=False -> S7ConnectionError: Connection closed by peer
S7-1200 1212C V4.7.3     use_tls=True  -> CONNECTED protocol_version=V2
S7-1500 1511F V2.9.7     use_tls=False -> S7ConnectionError: Connection closed by peer
S7-1500 1511F V2.9.7     use_tls=True  -> CONNECTED protocol_version=V2

Suggested fix: a --plc-use-tls flag (plus --plc-tls-cert/key/ca passthrough) on run_real_plc_acceptance.py, threaded into the adapter, and recorded in the report metadata since it is part of the tested security mode. Without it the acceptance program can only certify pre-V4.5 S7-1200s and V1 S7-1500s.

Results as they stand

protocol S7-1200 1212C V4.7.3 S7-1500 1511F-1 PN V2.9.7
legacy_s7 Establish and close a session — PASS Establish and close a session — PASS
s7commplus all fail at connect (no TLS) all fail at connect (no TLS)

The other four scenarios fail on both CPUs for a reason that is ours, not yours: neither PLC carries the DB1/DB2 fixture from tests/plc_setup/e2e_test_dbs.scl. Importing it needs TIA Portal and our automation engineer; it is requested and should land with his next session, after which I can run the full suite including --allow-write.

So this is a partial, and I would rather report it now than sit on the TLS finding.

One note on hardware scope

#881's outstanding item-read failure is on an S7-1511C. Ours is a 1511F — same S7-1500 family, different model and a safety variant. Whatever we produce is useful compatibility evidence but will neither confirm nor clear that specific report.

I will file the formal Real PLC test result issues with both artifacts once the fixture DBs are in and the TLS path is reachable — filing a result now would mostly record a harness gap rather than PLC behaviour.

@russwing

Copy link
Copy Markdown
Contributor

Fixture DBs are now in place on both CPUs, so here is the full pass. Two defects, one of them substantial, plus confirmation that the TLS fix works.

Harness eb6d9e3, plus a one-line local patch described below. Both CPUs are dedicated bench units.

Results

S7-1200 1212C V4.7.3 S7-1500 1511F-1 PN V2.9.7
legacy_s7 smoke 5/5 PASS 1/5 — PUT/GET is disabled (class=0x81 code=0x04, service not implemented). A CPU setting, not a library issue.
s7commplus smoke, stock 0/5 — cannot connect 0/5 — cannot connect
s7commplus smoke, TLS patched 1/5 — session PASSES, all reads fail 1/5 — session PASSES, all reads fail

1. The TLS fix works — confirmed

Patching S7CommPlusAdapter.connect to pass use_tls=True turns the connection scenario from fail to PASS on both CPUs. That confirms the fix proposed in my earlier comment. Please add --plc-use-tls (plus cert/key/ca passthrough) and record it in the report metadata.

2. db_read over S7CommPlus ignores offset and size

This is the bigger one, and it is not fixed by #881 — I merged #881 into the harness branch and re-ran; byte-for-byte identical failure on both CPUs.

Same CPU (1212C), same DB, same offsets, both protocol paths side by side:

DB1.0  size= 2   legacy=00 0a                     s7cp=92 00 00 01
DB1.0  size= 8   legacy=00 0a 00 ff 42 f6 e6 66   s7cp=92 00 00 01
DB1.2  size= 4   legacy=00 ff 42 f6               s7cp=42
DB1.0  size=16   legacy=<16 correct bytes>        s7cp=92 00 00 01

Three separate problems visible in four lines:

  • Size is ignored. Requests for 2, 8 and 16 bytes all return the same four bytes.
  • The data does not match the block. 92 00 00 01 appears nowhere in DB1's real contents.
  • Offset is ignored or mis-scaled. DB1.2 size=4 returns a single byte 42, which is the byte at DB1.4.

The legacy column is ground truth from the same CPU seconds apart, so the fixture and the DB are demonstrably correct.

Identical bytes on two different CPU families (1212C V4.7.3 and 1511F V2.9.7) points at client-side decode rather than anything CPU-specific.

On #881

You flagged an unresolved S7-1511C item-read failure and said a failure would be useful evidence. We reproduce a read failure on a 1511F and a 1212C, and #881 as it stands does not resolve either. Whether it is the same root cause as the 1511C report I cannot say — different model, and I do not have that reporter's fixture.

Given the size/offset behaviour above, I would treat this as distinct from "unprefixed response values": the transport is not returning a wrong-shaped version of the right data, it is returning the wrong data.

Scope notes

  • No --allow-write run yet. DB2 exists and writes are cleared on these bench units, but running scratch writes over a path whose reads are this wrong would tell us little and risks writing somewhere unintended. Happy to run writes once reads are trustworthy.
  • I have not filed formal Real PLC test result issues yet — the current results mostly characterise two library defects rather than PLC behaviour, and I would rather file a clean result once the TLS flag lands and reads are fixed. Say if you would prefer them filed as-is and I will.
  • PUT/GET on the 1500 can be enabled if you want the legacy path covered on that family; it is off by default there and the runbook rightly says not to weaken a PLC without cause.

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.

Formalize real-PLC acceptance testing with Gherkin scenarios

2 participants