Breadcrumb test plan has many commands with invalid key references

[mzgoddard]

Breadcrumb test plan has many commands with invalid key references. Some of these are just keys missing in keys.mjs, but some others like "DOWN,DOWN,DOWN" surrounded by quotes and containing commas, in this format cannot be added to keys.mjs. Key reference names currently need to be valid javascript variable names because of how they are stored in keys.mjs. "DOWN,DOWN,DOWN" could be renamed to something like DOWN_DOWN_DOWN without quotes and replacing the commas with underscores.

#536 has been opened to help make this more obvious during pull request review of test plan changes.

Breadcrumb errors from test generation:

*** 14 Errors in tests and/or commands in test plan [tests/breadcrumb] ***
[Command]: The key reference ""DOWN,DOWN,DOWN"" is invalid for the "navigate to first breadcrumb link" task.
[Command]: The key reference "SHIFT_I" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference ""UP,UP,UP"" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference "SHIFT_L" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference "SHIFT_I" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference ""CTRL_OPT_RIGHT,CTRL_OPT_RIGHT,CTRL_OPT_RIGHT"" is invalid for the "navigate to first breadcrumb link" task.
[Command]: The key reference ""CTRL_OPT_LEFT,CTRL_OPT_LEFT,CTRL_OPT_LEFT"" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference ""DOWN,DOWN,DOWN"" is invalid for the "navigate to first breadcrumb link" task.
[Command]: The key reference "SHIFT_I" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference ""UP,UP,UP"" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference "SHIFT_L" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference "SHIFT_I" is invalid for the "navigate to last breadcrumb link" task.
[Command]: The key reference ""CTRL_OPT_RIGHT,CTRL_OPT_RIGHT,CTRL_OPT_RIGHT"" is invalid for the "navigate to first breadcrumb link" task.
[Command]: The key reference ""CTRL_OPT_LEFT,CTRL_OPT_LEFT,CTRL_OPT_LEFT"" is invalid for the "navigate to last breadcrumb link" task.

[jscholes]

some others like "DOWN,DOWN,DOWN" surrounded by quotes and containing commas, in this format cannot be added to keys.mjs

@mzgoddard I'm going to need some clarification here. It was decided many months ago that the test format would be updated to support command arrays (#363). Such support was then implemented in PR #438, which was merged many months ago. The comma-separated commands in quotes should be split, and each treated as a command look-up in the keys file. Is that not the case?

[mzgoddard]

The comma-separated commands in quotes should be split, and each treated as a command look-up in the keys file. Is that not the case?

Ah, yes. That is there. For the code merged in #438, the beginning and ending double quotation mark from "DOWN,DOWN,DOWN" needs to be removed. As is it will be considered as "DOWN, DOWN, and DOWN". The quotes are not ignored by the test generation or that code.

Along with removing the quotes from the breadcrumb test plan commands.csv, we'll need to add the comma separation logic to the test generation to resolve the error messages.

[jscholes]

@mzgoddard

the beginning and ending double quotation mark from "DOWN,DOWN,DOWN" needs to be removed.

Sorry, I'm still confused. If we removed the quotes, the commas would be treated as column separators, because it's a CSV file. I.e. it would be three commands, not three repeated presses of Down Arrow to form a single one.

Looking at commands.csv, though, it looks like we've put in some incorrectly escaped quotes, i.e. the string starts and ends with """ (three quotes) instead of just a single quotation mark. So the actual sequence should be: "DOWN,DOWN,DOWN".

we'll need to add the comma separation logic to the test generation to resolve the error messages.

I thought that was the point of #438? Are you saying that the support for command sequences only applies to the generated JSON files, that nobody ever creates by hand, and that the automated pipeline can't actually create that structure in the first place?

[jscholes]

Update: the triple quotes in this commands.csv were exported by Excel trying to be smart.

[mzgoddard]

Sorry, I'm still confused. If we removed the quotes, the commas would be treated as column separators, because it's a CSV file. I.e. it would be three commands, not three repeated presses of Down Arrow to form a single one.

Update: the triple quotes in this commands.csv were exported by Excel trying to be smart.

Quotes are used in a kind of unique way in csv files. "DOWN,DOWN,DOWN" in an spreadsheet editor exported to csv should be formatted """DOWN,DOWN,DOWN""". The quotes are written as pairs of double quotes "" and the whole value is wrapped with a single beginning double quote and single ending double quote.

To remove the beginning and ending double quote characters that the test generation script sees in the commands.csv file, it should be written "DOWN,DOWN,DOWN" in the csv. In excel that would be written DOWN,DOWN,DOWN without any quotes. The value "DOWN,DOWN,DOWN" in the csv file will be parsed by excel and our test generation as DOWN,DOWN,DOWN.

I thought that was the point of #438? Are you saying that the support for command sequences only applies to the generated JSON files, that nobody ever creates by hand, and that the automated pipeline can't actually create that structure in the first place?

I reviewed the logic in the test generation and currently that is correct. It doesn't split a command containing commas before looking them up. I'm not sure if something slipped by us for this or if another PR added it and it got removed for another reason such as being part of a revert.

[jscholes]

@mzgoddard Thanks, this was supr helpful. No quotes in Excel seems obvious now, given that it's a single cell, and the program should/does just do the right thing when exporting.

On the second part, re: support for command sequences not actually being implemented in a usable way after all, that is disappointing, and will have a major impact on the test plans we've been writing. Since the relevant PR was merged, we've been writing all of our tests to comply with what we believed was the new spec, and also have some time carved out to revisit older plans which use different messaging.

I've reopened #363, because it currently can't be considered fixed. Do you have thoughts on how this work can be prioritised to support the tests as written, and when someone may be able to look at completing the implementation? CC @sinabahram / @s3ththompson / @richnoah

[jscholes]

I've fixed the quoting in #540.

[jscholes]

@mzgoddard Looking into this a bit more, I'm not sure why you think it's an issue. Looking at the Test plan review for Breadcrumb, the command sequences seem to be rendered as expected. E.g. for test 1, Navigate to the first breadcrumb link in reading mode, the page states:

... using the following commands:

• U
• I
• Tab
• Down Arrow, then Down Arrow, then Down Arrow

Granted, I think the wording could be better when there are three or more instances of the same command in a sequence. But that's pretty much how it was declared to be working in #438. They also seem to be working fine for the slider test plan that I just merged.

[jscholes]

I dunno, I'm just confused @mzgoddard. I've worked on a few test plans across pull requests today, and in all cases the checks were successful. Similarly, in all cases I merged master into the working branch before pushing. But as shown in https://github.com/w3c/aria-at/runs/3985448927?check_suite_focus=true, there are failures all over the place, including in those test plans that I've merged that were previously reported as having no failures.

In essence,, it seems like whether or not the build process communicates errors (or exits with a specific code) is completely inconsistent, which makes it impossible to develop tests with any degree of confidence. I'm going to leave it until I hear more from the Bocoup side.

[jscholes]

In PR #487, I fixed some legitimate test plan errors, i.e. mismatched task names across commands.csv and tests.csv. Those errors were in the ratings slider plan, introduced in PR #486. I merged that earlier PR, because it reported "5 / 5 checks OK" (as you can still see on the page). But after merging master into the newer PR for the temperature slider and pushing, all of a sudden the ratings slider test plan won't build.

[s3ththompson]

Per a recent conversation with @jscholes and @mzgoddard, the plan to address this issue is as follows:

• Ensure all test generation errors yield an exit code 1 to fix an issue where validation errors were being erroneously silenced (Exit with code 1 if a test plan produced errors #536)
• Add an alternate validation step for command sequences to the test generation script to remove erroneously-reported validation errors and make legitimate errors easier to spot (Support comma separated key sequences in test generation #563)
• Add command sequence support to generated .collected.json test files to ensure consistency with generated .html test files (Support comma separated key sequences in test generation #563)
• Ensure App understands how to import and render command sequences