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.

Sign up for GitHub

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

Jump to bottom

Test Refactor #145

Closed
spectranaut opened this issue Apr 1, 2020 · 9 comments
Closed

Test Refactor #145

spectranaut opened this issue Apr 1, 2020 · 9 comments
Labels
tests About assistive technology tests

Comments

@spectranaut
Copy link
Contributor

spectranaut commented Apr 1, 2020 •
edited
Loading

In consuming the tests, I've realized we need to do a refactor of the test format of the test .html files. The purpose of the refactor is to more easily consume metadata from the tests. The refactor is ultimately pretty small, but it will effect three things in this repository. The changes that will have to be made:

  • Jon's csv files to test files script (scripts/create-tests.js)
    • This change will be very straight forward, because the tests files have a template. The information is the same, but the template will have to change.
    • Changing this script will allow us to update all the test files to the new format in one go, which is very exciting!
  • The aria-at-harness.mjs file (tests/resources/aria-at-harness.mjs)
    • The logic is almost entirely the same, but the entry point to the logic will have to change. Instead of getting that information from the VerifyATBehavior call, the script will look for a <script id ="test-data" type="application/json"></script> and initiate the test from there.
    • The javascript setupTestPage code will have to be escaped to to include it in the JSON. Since these test files will most likely never be written by hand, I think that is reasonable. We will then have to inject it into the popup window with the APG example in a different way, maybe appending the popup html with a script tag with the stringified javascript inside of it.
  • The script that makes the "review test plan" pages (scripts/test-reviewer.mjs).
    • I think this script should be removed. Instead, I think Jon's script should make "review test plan" pages at the same time that it produces the test files.
    • If we don't remove this script, then we need to update it to consume the new test format instead of the old test format. Should be straight forward, as the change makes the data more structured and easy to parse.

And example of the new format I am imaging for the test files is provided at the end of this issue. For comparison, see the original format for this test in: read-checked-checkbox.html.

The change moves all the test specific information from a javascript script tag (where all the test specific information is passed to the harness through an API call) to a json script tag. When designing these test, it seemed like we might want to support a lot more logic in the test file, but it turns out the only javascript you need is pretty minimal. Moving all the declarative parts of the test to the JSON script tag I think simplifies the test file and makes it very straightforward to consume in the runner.

<!DOCTYPE html>
<meta charset="utf-8">
<title>Read a checked checkbox in reading mode.</title>
<link rel="author" title="Valerie Young" href="mailto:valerie@bocoup.com">
<link rel="help" href="https://w3c.github.io/aria-practices/#checkbox">
<link rel="help" href="https://w3c.github.io/aria/#checkbox">
<link rel="help" href="https://w3c.github.io/aria/#aria-checked">
<script id="test-data" type="application/json">
{
  "setup_script_description": "Mark the first checkbox as checked",
  "setupTestPage":
    "\\n      const checkboxes = testPageDocument.querySelectorAll('[role=\\"checkbox\\"]');\\n      checkboxes[0].focus();\\n      checkboxes[0].setAttribute('aria-checked', 'true');\\n",
  "applies_to": [
    "NVDA",
    "JAWS"
  ],
  "mode": "reading",
  "task": "read checkbox",
  "specific_user_instruction": "When the focus or reading cursor is on the first checkbox, read the first checkbox (which is now checked)",
  "output_assertions": [
    [1, "Role 'checkbox' is conveyed"],
    [1, "Name 'Lettuce' is conveyed"],
    [1, "State of the checkbox (checked) is conveyed"]
  ],
  "test_page": "reference/two-state-checkbox.html"
}
</script>
<script type="module" src="../resources/aria-at-harness.mjs">
@spectranaut spectranaut added the Agenda+Community Group To discuss in the next workstream summary meeting (usually the last teleconference of the month) label Apr 1, 2020
@zcorpan zcorpan added the tests About assistive technology tests label Apr 1, 2020
@mfairchild365
Copy link
Contributor

A potential benefit for moving the JSON to its own file - we could create a schema for the JSON structure and add that to our automated test suite for the project.

@zcorpan
Copy link
Member

zcorpan commented Apr 1, 2020

In the telecon today we discussed also removing author information, and rely on GitHub/git for attribution.

@mfairchild365
Copy link
Contributor

I'll take a look at this for the next call. Goal: estimate how long it will take me to finish this.

@zcorpan
Copy link
Member

zcorpan commented Apr 2, 2020

So with separate JSON files, the HTML files would be essentially identical. An idea is to have a single HTML file to run test, and use a hash in the URL to load the right JSON file. We could generate an index with links to all the "tests".

Something like

run-test.html#src=combobox.json

@mfairchild365
Copy link
Contributor

mfairchild365 commented Apr 2, 2020 •
edited
Loading

Do we have any security concerns around including executable JS in the JSON as described in the example? It would probably be best to avoid eval() if possible.

@zcorpan
Copy link
Member

zcorpan commented Apr 2, 2020

I think the JSON files can be trusted as much as the script file itself, but there should be validation on the src from the hash so it isn't able to fetch from an arbitrary URL.

@zcorpan
Copy link
Member

zcorpan commented Apr 2, 2020

It seems possible to avoid executable code in the JSON as well, e.g. by defining functions for all special cases needed and have the JSON data say which functions it needs.

@mfairchild365
Copy link
Contributor

It seems possible to avoid executable code in the JSON as well, e.g. by defining functions for all special cases needed and have the JSON data say which functions it needs.

That's the route that I was thinking. Its easier to read and just seems cleaner. The hard part will be generating a callable list of functions, which are defined separately for each test plan. Not quite sure how to approach that in a maintainable way; but I'll explore it.

@mfairchild365 mfairchild365 mentioned this issue Apr 3, 2020
Merged
4 tasks
@zcorpan
Copy link
Member

zcorpan commented Apr 22, 2020

I've merged the PR. The "single HTML file" idea wasn't implemented, and I started to look into doing that myself today. I think it might require changes to the prototype test runner as well? It might be more work than I thought at first. I suggest we drop the idea for now.

@zcorpan zcorpan removed the Agenda+Community Group To discuss in the next workstream summary meeting (usually the last teleconference of the month) label Apr 22, 2020
@zcorpan zcorpan closed this as completed Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
tests About assistive technology tests
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@zcorpan @mfairchild365 @spectranaut