test(conformance): add audited Workshop feature suite - #88
Conversation
Teakowa
left a comment
There was a problem hiding this comment.
CI is green, but this does not yet satisfy #87's documented-contract conformance goal.
Merge blockers:
-
The positive Action/Value cases currently use the implementation as their own oracle.
inventory_rowskeeps only Feature + Status, whilecall_program_sourcederives arity fromCatalogEntry.paramsand argument values fromparam_type/param_domain. If the catalog has the wrong parameter order/type/default/return contract, the test constructs input from that same wrong contract and can pass. The auditedactions.md/values.mdNotes already contain Returns/Parameters and should be asserted independently (or represented by an explicitly audited test contract that is not generated from the catalog under test). -
Enum/member completeness is also catalog-derived. The suite iterates
domain.members, so a documented member missing from the catalog cannot be discovered.enums.mdcurrently records counts plus examples, not the full leaf-member inventory. #87 requires per-member coverage; add a durable audited member inventory/contract and drive expected members from it, then compare/test the catalog implementation against that set. -
Several feature IDs do not exercise the named feature. For example
Set/Modify ... At Indexrows map to the genericvariables_source()rather than using the indexed construct, and the Settings summary counts all supported rows while executing onepixelartfixture. Each supported leaf row needs an attributable case that actually contains/exercises that construct; otherwise the ID is only a label. -
Negative conformance remains representative-only. There are only three invalid cases. For documented signatures, add data-driven rejection checks where applicable (at minimum wrong arity and concrete incompatible domain/type) so the suite guards the same contract it claims to validate.
The Return change to 🚧 Coming soon is a correct finding, but it also shows that #86's closed audit still had at least one false-positive Supported structural row. Before #87 closes, revalidate the non-catalog structural/settings rows against the audited contract rather than assuming #86's existing statuses are complete.
The target remains offline language semantics only: documented input/contract -> expected parse/validation/semantic/emission result. No live-game/runtime testing is needed.
6ff152b to
fbde2c9
Compare
fbde2c9 to
55b4eb5
Compare
Teakowa
left a comment
There was a problem hiding this comment.
Re-review of head 55b4eb56cb7596dfb22cdc466265b4d0b1b8fa05: the prior four blockers are substantially addressed, and CI run 32634660514 is green on stable + Rust 1.85 + real-project scenarios. Two contract-closure issues remain before I would consider #87 complete:
-
Documented parameter order is still not fully asserted.
assert_documented_contractcomparesparam_type(index)whenever the documented parameter has a type, and only compares the parameter label in the no-type branch. Since the audited Action/Value rows normally carry types, a swap of two same-typed parameters can remain green. Example: twoNumberparameters with distinct semantic labels/order. #87 explicitly requires documented parameter count/order. Compare the documented label againstentry.params[index]independently of the type check, then compare type/domain separately. -
Defaults/optional-argument semantics are still absent from the audited oracle/test path.
CatalogEntryhasparam_defaultsandrequired_param_count(), butDocumentedContractcurrently contains only return type + parameters, positive cases always supply the full parameter list, and negative arity only adds an extra argument. Thus a wrong default/required suffix can pass. #87 explicitly includes documented defaults where relevant. Either preserve audited defaults/optionality in the external contract and test omitted trailing defaulted arguments + required-argument rejection, or explicitly demonstrate that the #86 audited surface contains no such documented defaults. Existing catalog APIs show this is a real semantic dimension, not merely hypothetical.
One evidence note: audited-enum-members.md now fixes the previous implementation-derived member iteration, but the file only states that it is independently reviewed. Because #87 depends on #86's external inventory rather than catalog self-consistency, record enough provenance/derivation for reviewers to establish that this 733-row expected set did not simply come from the catalog under test. This can stay lightweight; it does not need a second public support database.
Everything else from the previous review looks materially improved: per-feature variable/subroutine/settings sources now exercise their named constructs, enum expected-vs-actual set comparison exists, and invalid Action/Value coverage is data-driven.
Fixes #87