test(grading): replay recorded browser traces, and stop crediting stale slots - #126
Draft
erseco wants to merge 10 commits into
Draft
test(grading): replay recorded browser traces, and stop crediting stale slots#126erseco wants to merge 10 commits into
erseco wants to merge 10 commits into
Conversation
…produce The plugin's grading path had no test that started from what a browser actually sends. `scorm_tracker.js` was covered by hand-written suspend_data literals, and `track::ingest()` by hand-built itemscores maps, so both sides could agree with each other while disagreeing with the package. Every grading defect found while auditing the SCORM channel was found by hand, and two of them had survived because no fixture reproduced the sequence that triggers them. **A trace replay harness.** `replayScormTrace()` feeds a recorded trace — the ordered API traffic of a real learner attempt, plus each page's `.idevice_node` ordering — through the real `js/scorm_tracker.js` and reports the itemscores it would POST. The tracker path is a parameter, so one trace can be replayed against two revisions and the results compared directly. Traces follow the versioned contract the exporter writes (see the core-side harness), so the two repos share a format instead of an implementation. Three oracle fixtures ship with it, each hand-computed from what the learner did: - a page-2 iDevice whose slot already holds a stale page-1 entry with the same score and weight — it must still be graded; - a stale entry that resolves against a later page's DOM — it must NOT be attributed to an iDevice the learner never answered; - a versioned `exe12/` payload, which the parsers previously read as empty. The first two are mirror images, and before this work each competing tracker passed one and failed the other. They are the specification of the attribution rule, which is why they ship with it. **A grading matrix for the server.** `grading_matrix_test.php` runs a declared roster and itemscores map through `track::ingest()` twice — PERITEM and OVERALL — and checks every published gradebook value against a hand-computed expectation, including the weighted cases (25/75 -> 25, 10/20/30/40 -> 30) and the fallbacks (no weights -> plain mean; partial reporting -> the visited-items denominator). Expectations are written by hand in the provider and never derived from the code under test. One recorded scenario shows why the assertions are on the per-item map rather than the total: a three-item case produced the arithmetically correct overall while silently dropping an iDevice, because (75+50+25)/3 equals (75+50+25+50)/4. A total-only assertion would have called it green. Companion to the core-side package generator and recorder: that branch builds the packages and records what an LMS receives, this one proves what the plugin then grades.
main took the objectid-keyed change baseline through #125; this branch carries the next step on top of it — the attribution test that stops a stale cross-page entry being credited to an iDevice the learner never answered. The three conflicts in js/scorm_tracker.js are that overlap, and resolve to this branch's version, which is a superset: it keeps the objectid-keyed baseline #125 introduced and adds the carry-over check around it. vitest 51/51 (4 files, replay suites included) · phpcs 0
This branch is stacked on #105 rather than on main: #105 carries the exe12 parsers it also contained, so basing it on main duplicated them and would have conflicted on merge. What is left here is what only this branch has — the trace replay harness, the recorded fixtures, the server-side grading matrix, and the attribution test that keeps a stale cross-page entry from being credited to an iDevice the learner never answered. The conflicts were that overlap. The cosmetic ones resolve to #105's wording so the stacked diff stays small; the one with substance is captureItemScores(), where this branch's body is a superset — same exe12 bypass, plus the isCarriedOver() check — so it keeps its body and gains #105's explanation of why a versioned entry skips DOM resolution. vitest 61/61 (4 files) · phpcs 0
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor/scorm12-runtime-clean-provenance #126 +/- ##
===============================================================================
+ Coverage 93.98% 94.03% +0.05%
+ Complexity 773 772 -1
===============================================================================
Files 47 47
Lines 3473 3487 +14
===============================================================================
+ Hits 3264 3279 +15
+ Misses 209 208 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Merging #105 into this branch appended a block of four test methods that were already present, byte for byte. PHP refuses to redeclare them, so the whole mod_exelearning PHPUnit suite failed to load — not one test red, the suite not starting at all, which is why nothing on this branch had been verified since. Cannot redeclare mod_exelearning\track_test:: test_parse_suspend_data_reads_the_versioned_exe12_payload() Removes the second copy of each: reads_the_versioned_exe12_payload, skips_unusable_exe12_records, ignores_an_unsupported_exe12_version and ingest_routes_a_versioned_suspend_payload_by_objectid.
The "Graded" switch used to silence the grade but not the recording: with it off, ingest() still wrote the attempt row, marked gradable = 0. Two reasons were given — completion by status reads that row, and it is the history a later switch-on republishes from — and holding the line between "stored" and "counts towards a grade" inside one table took four separate pieces: the gradable column, gradability inherited per attempt, poisoning the whole attempt when a session crossed the switch, and maxattempt skipping ungraded attempts. An ungraded activity is a plain resource, and a plain resource has nothing to record. ingest() now acknowledges the request and writes nothing: no attempt row, no grade, no event. Turning the switch on starts recording from that moment; nothing is recovered from before it, and losing that work is accepted. This makes DEC-124-03 — work done while ungraded never becomes a mark — true by construction instead of by a mechanism that has to hold. History recorded while the activity WAS graded is untouched, and DEC-124-01 still republishes it when the switch comes back on. Two consequences are accepted deliberately and recorded in DEC-126-01: completion by status does not resolve on an ungraded activity, and a learner who holds the page open across a switch-on has their accumulated scores graded by the first POST after it, because the client resends its whole map. The gradable column and its read-side filters stay: rows written by earlier versions, and restored backups carrying them, must keep being excluded from every aggregation. Nothing writes a 0 any more.
…o reads The matrix test declared its data provider with the PHPUnit 10+ attribute #[DataProvider]. Moodle 4.5 runs PHPUnit 9.6, which does not read attributes: the provider was ignored, the test method was called with no arguments, and the whole lane failed with ArgumentCountError while every Moodle 5 lane was green. The @dataProvider annotation is read by both, and is what the rest of this plugin's tests already use.
package_manager already replaces the two runtime files in every extracted package, so the bytes were never in doubt. The script tags were: an eXeLearning SCORM 1.2 export references libs/SCORM_API_wrapper.js and libs/SCOFunctions.js itself, and this plugin accepts such a package — content.xml at the root is the only thing it validates — so the injector's pair was added next to the package's own and the whole runtime was parsed and executed twice. Measured in a live Moodle before this change: an activity created from a SCORM export served a page carrying two wrapper tags and two SCOFunctions tags. The LMS-visible traffic survived it — one LMSInitialize, and one commit plus one finish at pagehide — but "it happens to be idempotent" is not a contract, and nothing was testing it. An ELPX, which references neither, was already fine. The injector now strips any script tag that loads either file before adding its own, so the page ends with exactly one of each, at the plugin's own relative depth. Same rule as the files themselves: the plugin's runtime wins, and there is only one of it.
The provenance tests checked the SHAPE of the vendored runtime: five layers, in order, stamped, with no local-change marker. None of that catches a copy with the wrong CONTENT. An edit inside a layer keeps every banner in place, keeps the stamp, and passes — which is exactly the drift the folder is supposed to be protected from. And the stamp cannot close the gap on its own: it names a release, and a release is built many times. assets/scorm/SOURCE now records the core repository, ref and full commit the copy was exported from, the version stamped into it, and the SHA-256 of both files. Two new tests read it: the files must match those digests, and the commit must be a full commit id rather than a branch name or a release string. Verified for the copy in the tree: re-exporting from the declared commit with `make export-scorm12` reproduces SCOFunctions.js byte for byte (sha256 567469a1…), and the wrapper is still the unmodified pipwerks file (f2a558ba…) recorded in THIRD-PARTY-NOTICES.md upstream.
…ng-trace-replay
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
2 participants
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.
Stacked on #105, which lands first and is merged into this branch, so the two are in sync.
Companion to exelearning#2310, which builds the packages and records the traces this replays.
What this is
The grading path had no test that started from what a browser actually sends.
scorm_tracker.jswas covered by hand-writtensuspend_dataliterals andtrack::ingest()byhand-built
itemscoresmaps, so both sides could agree with each other while disagreeing withthe package. Every grading defect found while auditing the SCORM channel was found by hand, and
some had survived precisely because no fixture reproduced the sequence that triggers them.
A trace replay harness
replayScormTrace()feeds a recorded trace — the ordered API traffic of a real learnerattempt, plus each page's
.idevice_nodeordering — through the realjs/scorm_tracker.jsand reports the itemscores it would POST. The tracker path is a parameter, so one trace
replays against two revisions and the results compare directly. That is how the state report
was produced:
Three oracle fixtures ship with it, each hand-computed from what the learner did:
exe12/The first two are mirror images, and each competing tracker design passes one and fails the
other. They are the specification of the attribution rule, which is why they ship with it.
The defect this fixes
mainand #105 both over-grade the stale-slot trace:objB2=40for an iDevice the learnernever answered. Measured, on both:
captureItemScores()gains an attribution test: a legacy entry is credited to the objectid itsslot resolves to unless an identical entry — same title, same score, same weight — is
already banked for a different objectid that the loaded page does not own, in which case it is
a carried-over copy and is ignored entirely, neither graded nor banked under the wrong owner.
A versioned
exe12/entry names its own activity and skips all of it.Two residual limits are documented where the rule lives, honestly: two iDevices on different
pages that share a title AND score AND weight are indistinguishable in the legacy format; and a
stale entry whose real owner was never banked cannot be recognised as stale.
A production change, declared: with grading off, nothing is recorded
This PR also changes what
track::ingest()does when the activity's Graded switch is off:it acknowledges the request and writes nothing — no attempt row, no grade, no event. Turning
the switch on starts recording from that moment, and nothing is recovered from before it.
Until now the switch silenced the grade but not the recording: the attempt row was still
written, marked
gradable = 0. Two reasons were given for that — completion by status readsthe row, and it is the history a later switch-on republishes from — and holding the line
between "stored" and "counts towards a grade" inside one table took four separate pieces: the
gradablecolumn, gradability inherited per attempt, poisoning the whole attempt when a sessioncrossed the switch, and
maxattemptskipping ungraded attempts.An ungraded activity is a plain resource, and a plain resource has nothing to record. This makes
DEC-124-03 — work done while ungraded never becomes a mark — true by construction instead of
by a mechanism that has to hold, and it removes the four pieces. Recorded as DEC-126-01,
which supersedes DEC-124-02 only: DEC-124-01 (republish from history on a grading-config change)
stays in force, and the history it republishes now contains only work done while the activity
was graded.
Two consequences are accepted deliberately and written down rather than discovered later:
the attempt row and there is none;
scores graded by the first POST after it, because the client resends its whole itemscores map
by design, so a failed POST cannot lose a mark. Reloading the page starts clean.
The
gradablecolumn and its read-side filters stay: rows written by earlier versions, andrestored backups carrying them, must keep being excluded from every aggregation. Nothing writes
a 0 any more. Removing the column is a migration and belongs in its own change.
The nine tests that encoded the previous contract were rewritten to this one, the branches in
classes/local/track.phpthat only an ungraded ingest could reach were removed, anddocs/GRADEBOOK.md,docs/TRACKING.mdanddocs/tracking-architecture.mdwere corrected.A grading matrix for the server
grading_matrix_test.phpruns a declared set of gradable iDevices and itemscores map throughtrack::ingest()twice — PERITEM and OVERALL — and checks every published gradebook value against a
hand-computed expectation:
Expectations are written by hand in the provider and never derived from the code under test —
verified by mutation: changing one expected value from 40 to 41 makes it fail.
Why assertions are on the item map
A three-item recorded scenario produced the arithmetically correct overall while silently
dropping an iDevice, because
(75+50+25)/3equals(75+50+25+50)/4. A total-only assertionwould have called it green.
Scope
This covers mod_exelearning.
mod_scormandmod_exescormwere compared by hand againstreal Moodle instances during the audit; automating them would need their own harness.
Tests
The PHPUnit figure is the whole plugin suite with #105 merged in, not just this PR's own tests.
Draft
Draft because #105 lands first. #105's current tip is merged into this branch, so what CI runs
here is the stack as it will land.
Moodle Playground Preview
The changes in this pull request can be previewed and tested using a Moodle Playground instance.
ℹ️ The eXeLearning editor is fetched from the shared release and unpacked into the plugin when the playground boots, so the first load may take a few extra seconds. ELPX upload, viewer and preview work normally.