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

Validation for test csv formats #980

Merged
merged 8 commits into from
Aug 23, 2023
Merged

Validation for test csv formats #980

merged 8 commits into from
Aug 23, 2023

Conversation

gnarf
Copy link
Contributor

@gnarf gnarf commented Aug 16, 2023 •
edited
Loading

Preview Tests

  • Adding some additional validation for the various csv formats in the tests folder.
  • Moved create-example-tests to process-test-directory in the lib folder
  • I had attempted to avoid committing the output html changes to make review easier, but the automation bot did it anyway -- Here is a quick link to review with only the code changes (before the automation regenerated htmls) https://github.com/w3c/aria-at/pull/980/files/4dd172ae4271b2f804323da0281a0b5803185486

Some example errors from testing various ways of breaking commands.csv

typo in commands.csv header row:

WARNING: Error parsing F:\Code\aria-at\aria-at\tests\alert\data\commands.csv row 1: Error: Unknown commands.csv key: commsandA - check header row?

typo in command.csv command

[Command]: The key reference "SPACEs" found in "tests\alert/data/commands.csv" for "test id 1: trigger alert" is invalid. Command may not be defined in "tests/resources/keys.mjs".

typo in commands.csv task name

*** 2 Errors in tests and/or commands in test plan [tests\alert] ***
[Test 1]: command is missing for the combination of task: "trigger alert", mode: "reading", and AT: "jaws"
[Test 1]: command is missing for the combination of task: "trigger alert", mode: "reading", and AT: "jaws"

Mismatch of columns between entries in CSV:

WARNING: Error parsing F:\Code\aria-at\aria-at\tests\minimal-data-grid\data\tests.csv line 37: column number mismatch, please include empty cells to match headers. Expected 17 columns, found 16

@gnarf gnarf requested a review from howard-e August 16, 2023 16:51
@gnarf gnarf force-pushed the gnarf-validation-wip branch from 3b85c16 to 4dd172a Compare August 16, 2023 18:51
@gnarf gnarf force-pushed the gnarf-validation-wip branch from 0b5d9b4 to c4ee792 Compare August 21, 2023 16:47
@gnarf gnarf marked this pull request as ready for review August 21, 2023 23:39
@gnarf gnarf self-assigned this Aug 21, 2023
@gnarf gnarf requested a review from jugglinmike August 21, 2023 23:39
jugglinmike
jugglinmike reviewed Aug 22, 2023
View reviewed changes
lib/data/process-test-directory.js Outdated Show resolved Hide resolved
lib/data/process-test-directory.js Outdated
const firstRowKeysLength = Object.keys(rawCSV[0]).length;
for (; index < rawCSV.length; index++) {
if (!rowValidator(rawCSV[index])) {
printError('validator returned false result');
Copy link
Contributor

Choose a reason for hiding this comment

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

The validators signal failure by throwing errors, so this branch seems unreachable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this was a secondary idea to catch any other validation situation, then I made it throw errors for better output. I still think an undefined or falsy result from this "validator" should be an error, though none of our code is using this path currently

lib/data/process-test-directory.js Show resolved Hide resolved
gnarf and others added 2 commits August 23, 2023 11:42
@gnarf @jugglinmike
correct folder name in comment
63d262d 
Co-authored-by: jugglinmike <mike@mikepennisi.com>
@gnarf
special error for numeric key format
7b671ce
@gnarf gnarf force-pushed the gnarf-validation-wip branch from 2643375 to 7b671ce Compare August 23, 2023 16:01
@gnarf gnarf merged commit 836fb2a into master Aug 23, 2023
@gnarf gnarf deleted the gnarf-validation-wip branch August 23, 2023 20:30
@mcking65 mcking65 mentioned this pull request Aug 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jugglinmike jugglinmike jugglinmike left review comments

@howard-e howard-e Awaiting requested review from howard-e

Assignees

@gnarf gnarf

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gnarf @jugglinmike