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

Add harness input/output #466

Merged
merged 10 commits into from
Aug 31, 2021
Merged

Add harness input/output #466

merged 10 commits into from
Aug 31, 2021

Conversation

mzgoddard
Copy link
Contributor

@mzgoddard mzgoddard commented Aug 5, 2021

Preview Tests

  • Add TestRunInputOutput class to gather various test information in a test html file.
  • Move classes out of aria-at-harness so they can be used by dependents of aria-at.

TestRunInputOutput Summary

The main change in this PR adds the TestRunInputOutput class. It takes Input helpers instances to produce the state for a TestRun instance and options for a TestWindow instance. Each Input type has a public interface representing some current information processed currently at runtime.

This PR includes static methods on those inputs that process currently available information in the test html files for that public interface. This design is intended to provide a means to iterate the input data format for a test. The current data has a diverse set of format and is collected in many distinct methods, making it difficult to use and understand.

After this PR we will add new implementations of the Input types and process improved data formats while maintaining backwards compatibility with the existing formats.

New Files

  • aria-at-test-io-format.mjs
    • class KeysInput
    • class SupportInput
    • class CommandsInput
    • class ConfigInput
    • class ScriptsInput
    • class UnexpectedInput
    • class TitleInput
    • class BehaviorInput
    • class PageUriInput
    • class TestRunInputOutput
    • class TextRunExport
  • aria-at-test-window.mjs
    • class TestWindow

Inputs

TestRunInputOutput currently consumes 9 Input types which can be broken into two groups: standalone and dependent. Standalone inputs can be processed on their own. Dependent inputs require one or more other inputs to be able to process their information.

  • TitleInput, a standalone input. The title is the title of the test and originally appears in tests.csv. In this PR it is sourced from a test html file's title element.
  • PageUriInput, a standalone input. The page uri is the reference page loaded in the test window and originally appears in references.csv. In this PR it is provided as an argument to one of the aria-at-harness functions.
  • UnexpectedInput, a standalone input. The set of unexpected behaviors listed in the instruction document. In this PR it is builtin aria-at-test-io-format.
  • ScriptsInput, a standalone input. The set of executable scripts currently used to setup the reference page after it is loaded in a test window. In this PR it is fetched with the global scope member scripts if it exists.
  • SupportInput, a standalone input. The support information provided by tests/support.json.
  • ConfigInput , a dependent input. The configuration sets things like which at a test is being performed with for tests that are defined for multiple ATs. In this PR the config comes from the page's url query parameters and depends on SupportInput.
  • KeysInput, a dependent input. The keys are a map of key ids to keystrokes a user will perform and some other key related data. In this PR this keys comes from the keys module and depends on ConfigInput. Importantly keys does not depend on SupportInput. The keys implementation in this PR could be provided a ConfigInput that does not need SupportInput to be instantiated.
  • CommandsInput, a dependent input. CommandsInput represents the information currently found in commands.json. In this PR it depends on ConfigInput and KeysInput to process the information provided by commands.json.
  • BehaviorInput, a dependent input. BehaviorInput represents the information currently found in a test.json file. In this PR it depends on CommandsInput, ConfigInput, KeysInput, TitleInput, and UnexpectedInput to process the information in a test.json object.

Processed Outputs

When TestRunInputOutput has the right inputs provided it has some methods that can be called to provide data in a format need to render test instructions, record test results, open a test window, and create the test result submission object.

  • testWindowOptions() depends on BehaviorInput, PageUriInput, and ScriptsInput. It returns the options for a TestWindow instance.
  • testRunState() depends on BehaviorInput and ConfigInput. It returns an initial state for TestRun instances.
  • submitResultsJSON(state) depends on BehaviorInput. It takes a current state from TestRun instances, and returns the json object submitted for a aria-at test.

@mzgoddard mzgoddard requested review from alflennik and howard-e August 5, 2021 14:41
@mzgoddard mzgoddard marked this pull request as draft August 5, 2021 14:41
@mzgoddard mzgoddard marked this pull request as ready for review August 5, 2021 17:56
Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

Having worked a bit closer with the implementation this has been built up on, I can confirm what here works quite well and I can certainly appreciate the separation of concerns this has introduced to aid in the developer experience when expanding up on what is here. This continues to be quite impressive to me!

After working a bit closely with this however, there are a few things I'd like to point out after the fact which this PR may not directly be addressing (apologies there).

  • Unless that explanation has slipped me around the input for /tests/aria-at-test-run.mjs, it seems I'm unable to provide the headers for the Record Results section -> Assertions table. It also appears it's getting defaulted to empty strings when the current defaults seem to be Assertion, Success case & Failure cases. See L:220
  • The deep nesting within some of the outputs such as the result of TestRun.testPage() is continues to be somewhat unsettling. For example, if I wanted, I could make a call to new TestRun({ ... }).testPage().instructions.instructions.instructions.commands.commands to retrieve the list of AT commands to be ran for the respective Test. It's somewhat excessive but I understand why it has to be like this. I do hope however that further simplification is in the future.

Other than that, general expectations of use/use cases could be further detailed to help onboard others when needed. An example of such would be in a previous meeting we had where it was explained that implementing TestWindow to then pass the resulting hooks to TestRun.

tests/resources/aria-at-harness.mjs Outdated Show resolved Hide resolved
@howard-e
Copy link
Contributor

howard-e commented Aug 12, 2021

@mzgoddard /tests/aria-at-test-run.mjs L:366

The default click event seems to using the incorrect event for the result.

hooks.setCommandAssertion({commandIndex, assertionIndex, result: AssertionResultMap.FAIL_MISSING}) -> hooks.setCommandAssertion({commandIndex, assertionIndex, result: AssertionResultMap.FAIL_INCORRECT})

@alflennik
Copy link
Contributor

Looks good Z! I still want to look a little bit more at the code level stuff, but what I can confirm is that having new JS APIs to interact in the app is huge, and you deserve a big pat on the back for that 😜 because it was a bit difficult before to imagine wrapping the test renderer.

mzgoddard and others added 7 commits August 23, 2021 10:12
A test run html file currently has data describing the test in 9 objects
and places. This library TestRunInputOutput accepts that data from its
separate sources and produces a state that TestRun can start with and a
result JSON to submit after all the results are collected.

- Handle importing the various import files and data bits
  - The support json file
  - The commands json file
  - The test behavior json file
  - The keys module
  - The query params configuration
  - The test title from document.title
  - The builtin unexpected behaviors list
  - The test page uri to open in a window passed as an argument
  - The scripts map object in the global scope

- Handle exporting to the TestRun class and JSON submission format
@mzgoddard mzgoddard force-pushed the mzgoddard-harness-io-formatting branch from ec6dcde to 36bc1bc Compare August 23, 2021 14:25
@mzgoddard mzgoddard force-pushed the mzgoddard-harness-io-formatting branch from 865d27c to 08d5c5f Compare August 23, 2021 15:04
@mzgoddard mzgoddard force-pushed the mzgoddard-harness-io-formatting branch from 108ca74 to f91bba1 Compare August 23, 2021 15:28
Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

Looks good Z! I've gotten to play around with this a lot in the revised ARIA-AT App, and it's been working great.

@howard-e
Copy link
Contributor

howard-e commented Aug 26, 2021

Everything looks great to me!

The only thing that stands out to me is not having the associated label tied to the AT's Output textarea input on the test page.
At /tests/aria-at-harness.mjs:commandResult:L:334, perhaps a forInput could be added for the label and the id attached for the following textarea.

@mzgoddard
Copy link
Contributor Author

The only thing that stands out to me is not having the associated label tied to the AT's Output textarea input on the test page.
At /tests/aria-at-harness.mjs:commandResult:L:334, perhaps a forInput could be added for the label and the id attached for the following textarea.

Noted. @howard-e Do you think it Is ok if I do that in another PR since that's in harness and this PR doesn't change the rendering behavior?

@howard-e
Copy link
Contributor

Agreed. That would be more appropriate.

@mzgoddard mzgoddard merged commit f2f2087 into master Aug 31, 2021
@mzgoddard mzgoddard deleted the mzgoddard-harness-io-formatting branch August 31, 2021 15:06
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.

3 participants