Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support v2 test commands #32

Merged
merged 13 commits into from
Dec 20, 2023
Merged

Support v2 test commands #32

merged 13 commits into from
Dec 20, 2023

Conversation

stalgiag
Copy link
Contributor

@stalgiag stalgiag commented Dec 4, 2023

This work adds support to the harness for v2 test output. My work here is limited by my lack of access to a Windows machine so I will lean on my reviewer(s) for verification that this works.

To test my work locally, I ran a mock agent. I was able to get identical output from identical v2/v2 tests:

  • navigate-forwards-to-a-mixed-checkbox-reading-nvda.collected
  • read-information-about-slider-reading-nvda.collected
  • navigate-backwards-by-ten-options-in-listbox-popup-interaction-nvda.collected

As part of this work, I added validation to the mock agent as well which necessitates an update to the testing snapshots as the validation writes logs around the PAGE_UP commands built into the earlier test files. The test coverage remains the same after these updates.

@stalgiag stalgiag requested a review from gnarf December 4, 2023 17:19
src/agent/driver-test-runner.js Outdated Show resolved Hide resolved
@@ -41,7 +41,8 @@
* @property {string} [commands[].extraInstruction] human-readable additional instruction to follow
* @property {object[]} assertions[]
* @property {1 | 2} assertions[].priority
* @property {string} assertions[].expectation
* @property {string} [assertions[].expectation] - Expected result, required if assertionStatement is not provided.
* @property {string} [assertions[].assertionStatement] - Assertion statement, required if expectation is not provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not reflected in the canonical definition of the type, contradicting the single-line comment which precedes this multi-line comment. @howard-e can you see about updating ARIA-AT as necessary? Does this need to be reflected in the Test Format V2 design document?

Copy link

@howard-e howard-e Dec 12, 2023

Choose a reason for hiding this comment

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

contradicting the single-line comment which precedes this multi-line comment

expectation is a derived value that came from the assertions found in a v1 tests.csv files format. expectation is synonymous with assertionStatement in v2 and could technically still be referenced through the expectation value but keeping them separate is preferred (to me) since it will line up with a known *.csv property. The comment indicates that this is the case, kinda. But it also makes it seem like there could be a case where expectation and assertionStatement could exist simultaneously for a version of a single test plan.

@howard-e can you see about updating ARIA-AT as necessary?

Can do!

Does this need to be reflected in the Test Format V2 design document?

I think my above comment might not require this but just to be clear, on reflecting assertionStatement?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my above comment might not require this but just to be clear, on reflecting assertionStatement?

Yup, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I attempted to clarify the comments to make it clear when to expect which and that they exist exclusive of one another. Let me know if there is any other change needed in this repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Follow-up: w3c/aria-at#1025

src/agent/mock-test-runner.js Show resolved Hide resolved
@stalgiag stalgiag requested a review from jugglinmike December 13, 2023 21:31
Copy link
Contributor

@gnarf gnarf left a comment

Choose a reason for hiding this comment

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

LGTM - Merging

(nevermind, saw change request from @jugglinmike , gonna let him review and merge)

jugglinmike added a commit to bocoup/aria-at that referenced this pull request Dec 20, 2023
This change was originally proposed and accepted in the
aria-at-automation-harness repository (which maintains a copy of the
type definition) [1].

[1] w3c/aria-at-automation-harness#32 (comment)
Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

I've validated this on my Windows system. Thanks, Stalgia!

@jugglinmike jugglinmike merged commit 1d8387c into main Dec 20, 2023
6 checks passed
@jugglinmike jugglinmike deleted the support-v2-test-commands branch December 20, 2023 23:57
howard-e pushed a commit to w3c/aria-at that referenced this pull request Dec 21, 2023
This change was originally proposed and accepted in the
aria-at-automation-harness repository (which maintains a copy of the
type definition) [1].

[1] w3c/aria-at-automation-harness#32 (comment)
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.

4 participants