Skip to content

fix: Correct testaro motion test to make an image comparable to the catalog image - #135

Open
jrpool wants to merge 5 commits into
mainfrom
motionbugs
Open

fix: Correct testaro motion test to make an image comparable to the catalog image#135
jrpool wants to merge 5 commits into
mainfrom
motionbugs

Conversation

@jrpool

@jrpool jrpool commented Aug 23, 2026

Copy link
Copy Markdown
Member

The test of the motion rule of the testaro rule engine delivers false positive results if the tested page contains details elements that are closed, because the test compares an image of the page as it exists with the catalog image, which was made only after all details elements were forced open.

This change corrects that error by opening all closed details elements before the motion image is made. If the expansion fails, the test is aborted with a relevant error message.

The same test also reports the fraction of all pixels that changed, but the denominator is the pixel count of the entire page. A typical page has a background of uniform color that is covered only fractionally by foreground content. If all of that content disappears or is replaced with other content in situ, the perceived change is probably about 100%, but the reported change may be about 2%. Pending a more sophisticated approximation of perceived change, the revised test reports the count of changed pixels instead of the fraction, and the ordinalSeverity computation is now based on that count.

Since the motion test now opens closed details elements, it is reclassified as contaminating in tests/testaro.ts and is moved to be adjacent to other contaminating tests.

@jrpool jrpool added the bug Something isn't working label Aug 23, 2026
@jrpool
jrpool requested a lite review from Copilot August 23, 2026 19:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the testaro motion rule to avoid false positives caused by <details> elements being forced open for the catalog screenshot but remaining closed for the motion comparison screenshot. It also changes reporting/severity to use an absolute changed-pixel count instead of a page-sized fraction, and reclassifies the rule as contaminating so it runs in the correct phase.

Changes:

  • Open all closed <details> elements before taking the motion comparison screenshot.
  • Report changed pixel count (and compute ordinal severity from that count) instead of reporting a percent-of-page change.
  • Mark motion as contaminates: true and reposition it among contaminating rules in the Testaro rule registry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/testaro.ts Reclassifies motion as contaminating and moves it into the contaminating section of allRules.
tests/testaro.js Mirrors the motion contaminating classification/order change in the emitted JS registry.
testaro/motion.ts Opens closed <details> before the screenshot; switches violation text and severity model to pixel-change counts.
testaro/motion.js Mirrors the motion.ts behavior changes in the emitted JS output.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread testaro/motion.ts Outdated
Comment thread testaro/motion.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread testaro/motion.ts
Comment thread testaro/motion.js
@jrpool
jrpool requested a review from wittjeff August 23, 2026 21:32
@wittjeff

Copy link
Copy Markdown
Collaborator

I appreciate the PR here. I'm a little confused on some of the logic bits, will need a bit more time to run it down. Sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants