Refactor of scripts/import-tests #291

howard-e · 2021-05-17T15:38:25Z

To continue the migration process, this holds changes for scripts/import-tests/index.js which is responsible for ensuring the latest tests can be pulled from the aria-at repository.

Assuming your database has not been initialized, run:

if [[ "$OSTYPE" == "darwin"* ]]; then
    createdb # run this if the PostgreSQL installation is freshly installed
    yarn db-init:dev;
else
    sudo -u postgres yarn db-init:dev;
fi;

yarn sequelize db:migrate;
yarn sequelize db:seed:all;
yarn db-import-tests:dev;

Changes:

test_version & apg_example INSERT instructions and attributes have now been converted to top level TestPlan records. Other additional attributes of those original tables have also been moved into TestPlan.parsed.
test has mostly been converted into ~~TestPlan.parsedTest.sourceSteps~~ TestPlan.parsed.tests to detail the relevant assertion tests to be done.
The amount of tests is now equal to TestPlan.parsed.maximumInputCount
test_version + apg_example + test -> TestPlan

test_version [OLD]

id	git_repo	git_tag	git_hash	git_commit_msg	datetime	active
1	https://github.com/w3c/aria-at.git	null	e7212c4e ...	Create tests for ...	2021-03-25 05:00:00.000000 +00:00	false

apg_example [OLD]

id	directory	name	test_version_id	active	example	design_pattern
1	checkbox	Checkbox Example (Two State)	1	false	https://w3c.github.io/aria-practices/examples/checkbox/checkbox-1/checkbox-1.html	https://w3c.github.io/aria-practices/#checkbox
2	checkbox-tri-state	null	1	false	https://w3c.github.io/aria-practices/examples/checkbox/checkbox-2/checkbox-2.html	null
3	...	...	...	...	...	...

test [OLD]

id	name	file	execution_order	apg_example_id	test_version_id
1	Navigate to an unchecked checkbox in reading mode	tests/checkbox/test-01-navigate-to-unchecked-checkbox-reading.html	1	1	1
2	Navigate to an unchecked checkbox in interaction mode	tests/checkbox/tests/checkbox/test-02-navigate-to-unchecked-checkbox-interaction.html	2	1	1
3	...	...	3	1	1
4	...	...	4	1	1

TestPlan [NEW]

id	title	publishStatus	sourceGitCommitHash	sourceGitCommitMessage	exampleUrl	createdAt	parsed
1	Checkbox Example (Two State)	...	e7212c4e ...	Create tests for ...	https://w3c.github.io/aria-practices/examples/checkbox/checkbox-1/checkbox-1.html	2021-03-25 06:34:12.000000 +00:00	See TestPlan.parsed example below

TestPlan.parsed Example

{
  "gitRepo": "https://github.com/w3c/aria-at.git",
  "directory": "checkbox",
  "tests": [
    {
      "file": "tests/checkbox/test-01-navigate-to-unchecked-checkbox-reading.html",
      "name": "Navigate to an unchecked checkbox in reading mode",
      "executionOrder": 1
    },
    {
      "file": "tests/checkbox/test-02-navigate-to-unchecked-checkbox-interaction.html",
      "name": "Navigate to an unchecked checkbox in interaction mode",
      "executionOrder": 2
    },
    {
      "file": "tests/checkbox/test-03-navigate-to-unchecked-checkbox-interaction.html",
      "name": "Navigate to an unchecked checkbox",
      "executionOrder": 3
    },
    ...
  ],
  "designPattern": "https://w3c.github.io/aria-practices/#checkbox",
  "maximumInputCount": 26,
  ...
}

…; removed sequelize seeders that are no longer relevant

alflennik · 2021-05-17T16:03:49Z

I really like the way you showed changes to the entities!

Something we should all talk about is the structure of the parsed tests. I think I saw Seth scheduling a meeting today to talk about it.

I sort of pulled the name "sourceSteps" out of my ... head, and what I was imagining is that a step would be part of a parsed test. It's probably not the best name. I think I imagined a step as being equivalent to a command - but it's basically one interaction a tester would do during a test.

Anyway that means the way you're using sourceSteps here doesn't match. Your parsed test is my test plan and your source steps is my parsed tests.

The solution in my mind is to figure out the structure of generated tests.

But that said I think this is mainly a terminology issue and I'm sure this PR won't have to change heavily. But I think it does reveal how important figuring out the test JSON is.

howard-e · 2021-05-17T16:11:25Z

Thanks!

And agreed, think we may just be off center in our interpretation of the naming there but the value stands. And definitely up for continuing to discuss those properties then.

Should make it a point to discuss TestResult.result in that same meeting as well then as that may rely on the structure of TestPlan.parsedTest as well.

…sedTest.testActions`

howard-e · 2021-05-17T18:15:00Z

But that said I think this is mainly a terminology issue and I'm sure this PR won't have to change heavily. But I think it does reveal how important figuring out the test JSON is.

@alflennik renamed to testActions for now to hopefully help with further discussions

alflennik · 2021-05-17T18:16:52Z

Sounds good @howard-e - and our meeting is in 15 minutes and I'll report back here if we make any decisions that are relevant!

alflennik

Looks awesome, I didn't get totally through it all yet, but here are my questions, almost all of which just come from my anxieties about the open modeling questions! But looks good.

alflennik · 2021-05-17T19:22:31Z

server/scripts/import-tests/index.js

+     * @param {string} testName - the name of the test
+     * @param {string} file - the relative path to the test file in the repository (ideally {@link https://github.com/w3c/aria-at.git})
+     * @param {number} testPlanId - TestPlan.id to be queried to update the TestPlan.parsedTest.testActions if necessary
+     * @param {string} commitHash - the hash of the latest version of tests pulled from the repository (ideally {@link https://github.com/w3c/aria-at.git})


I think we should make sure this matches the TestPlan model, so sourceGitCommit.

Given the fact we need the message too, it could be sourceGitCommitHash, in which case I'd definitely prefer to update the model from sourceGitCommit to sourceGitCommitHash as well.

I was puzzled at first because revision is being used as the hash here and sourceGitCommit as the message. But I follow now. I agree.

Re-purposing revision to be the latest tag found for the commit being pulled (may need to look specifically for semver-ed patterns)

sourceGitCommit -> sourceGitCommitHash

Adding sourceGitCommitMessage to the TestPlan schema

Awesome! Thanks for adding the message to the TestPlan too, that's a great solution.

As for revision, I think the hope is for it to come from a future version of the ARIA-AT repo, but it currently doesn't exist, haha. Maybe it will be stored with the test source as metadata on the test plan or something. Anyway, maybe the right move is to remove it until it's actually available.

Yes, right now we only have the git commit hash and message to track changes. if we include revision (for future-proofing) I think it should just duplicate the commit hash (since git tags are not really used by the aria-at repo). or, we can remove revision and add with a migration later.

alflennik · 2021-05-17T19:24:57Z

server/scripts/import-tests/index.js

+     * @param {string} file - the relative path to the test file in the repository (ideally {@link https://github.com/w3c/aria-at.git})
+     * @param {number} testPlanId - TestPlan.id to be queried to update the TestPlan.parsedTest.testActions if necessary
+     * @param {string} commitHash - the hash of the latest version of tests pulled from the repository (ideally {@link https://github.com/w3c/aria-at.git})
+     * @param {string} commitMsg - the message of the latest version of tests pulled from the repository (ideally {@link https://github.com/w3c/aria-at.git})


If you agree about the sourceGitCommit I think it would make sense to change this to sourceGitCommitMessage.

About msg, one convention I always fight for is to spell out words!!! I think it reduces guesswork and makes variable names easier to remember. I fight for it, but that's definitely an optional suggestion!

alflennik · 2021-05-17T19:29:47Z

server/scripts/import-tests/index.js

            );
+            // short circuit method because parsedTest.testActions.[action] is already present
+            if (testStepsFound) return testPlan.id;


Instead of testActions can I suggest commands? That matches a convention Simon and I sort of settled on a couple months ago.

I think in this case command is better than testCommand I want to avoid the trap of putting test in front of every variable name, haha, except in the case where we're too late as in TestPlan.

Doesn't commands in their context refer to the actual keyboard input commands being done?

Yes, in the current code, commands sometimes refers to the keys, but that's definitely something I want to change. And it's not just keys, it will encompass gestures on mobile devices too. In the CG when we touched on this terminology problem, the term "input" came up, which I think fits really well. So I want to gradually rename anything related to input to the name input, and I want to save the term command for the case we're dealing with here, the statements that make up a single test.

input -> the keyboard key or gesture
command -> columns in the test authoring CSV that contains either an instruction or an assertion

If I'm understanding correctly, I think these are just tests: https://github.com/w3c/aria-at/wiki/Working-Mode#definitions

Using this page as an example:

The hierarchy is: Test Plan ("checkbox") > Test ("Navigate to an unchecked checkbox in reading mode ") > Commands ("X / Shift+X") > Assertion ("Role 'checkbox' is conveyed")

That's right Seth - Howard and I talked about it yesterday and that was the conclusion.

sounds good! And seems like it might have implications on the name of the field as well? TestPlan.parsedTest should probably be closer to TestPlan.parsed since the JSON object is referring to the TestPlan as a whole, right?

Yes, that's right.

alflennik · 2021-05-17T19:30:36Z

server/scripts/import-tests/index.old.js

@@ -0,0 +1,446 @@
+/* eslint no-console: 0 */


If this is just a copy of the old version I think we can delete this and go back to the Git history if we ever need it, what do you think?

alflennik · 2021-05-17T19:31:06Z

server/scripts/import-tests/index.js

@@ -37,27 +37,24 @@ const tmpDirectory = path.resolve(__dirname, 'tmp');
 const testDirectory = path.resolve(tmpDirectory, 'tests');
 const supportFile = path.resolve(testDirectory, 'support.json');

-const ariaat = {
+const ariaAtImport = {


Thank you for changing that name, haha, this is much better.

Small (perhaps nitpicky) thing: we use "main" as our primary branch, not "master."

Right, will adjust

Actually, not seeing the main branch for aria-at. And master is still set as default so I suppose that branch rename could be proposed there?

I'll change the verbiage here to reflect a 'default branch' and set a constant for its use here should that change occur

alflennik · 2021-05-17T19:33:12Z

server/scripts/import-tests/index.js

+        };
+
+        const exampleUrl = getReferenceUrl(exampleRefLine);
+        const designPattern = getReferenceUrl(practiceGuidelinesRefLine);


I've sort of imagined exampleUrl as replacing the design pattern. Is it possible to eliminate the design pattern here?

So the exampleUrl and the APG designPatternUrl are different properties (example 1) and (example 2), so I'm not entirely sure we want to with that stance. But if there is a reason to no longer have one, that could be discussed.
However, given that the designPattern lives inside the json object and isn't tied to anything, we could also ignore it for now should it become useful.

Well, I do know we're going to talk about the references.csv pretty soon as one of the first topics of the larger test authoring format discussion. So yes, I think ... we should remove it.

I don't think we need to get rid of it unless we don't use it at all. The design pattern is a term that relates to the types of components in the example. See this issue as a reference: w3c/aria-at#402. I think it makes more sense to keep it as it is something that is heavily used in APG. As long as nothing is dependent upon it we can use or not use it as the examples indicate.

I agree with @jesdaigle that we should not get rid of designPatternUrl.

There are multiple examples for each APG design pattern. For example, "Checkbox Example (Two State)" is one of two examples under the "Checkbox" design pattern. Each URL has different, but useful contextual information. The CG wants to make it easy to visit either of these links for context for reviewers and testers.

For now, I think we should just track references.csv.

In the future, I agree with @alflennik that we should advocate for APG-agnostic metadata. This could be accomplished by a rename like: APG Pattern URL → Context URL or something like that, or it could be accomplished by a freeform json object that has different properties for each source that we're testing (APG, HTML, etc.). We should definitely discuss the pros/cons of these down the road.

Thanks for that context @jesdaigle and @s3ththompson I didn't realize the test plans had a hierarchy like that!

…Commit renamed to TestPlan.sourceGitCommitHash and adding TestPlan.sourceGitCommitMessage

…ult branch

s3ththompson · 2021-05-18T14:26:51Z

@howard-e @alflennik The meeting yesterday was more about the script that generates test files from the CSV files, rather than the actual structure of the test data.

As I commented above, I think we want to take a cautious approach to renaming things. Some terms (like "Test Plan", "Test", and "Assertion" are quite established and I don't think we should rename without very good reason). Other terms (like "APG Pattern URL" are definitely too specific and will need to be renamed in the future). For now, I think it's best to only tackle renames that would be hard to do later (or to treat certain test data as opaque JSON).

* Renamed `TestPlan.parsedTest` -> `TestPlan.parsed` * Renamed `TestPlan.parsed.testActions` -> `TestPlan.parsed.tests`

howard-e added 3 commits May 14, 2021 18:36

refactored import-tests script to conform with new database structure…

e5e0b07

…; removed sequelize seeders that are no longer relevant

recording maximumInputCount as number of steps for test

9e41739

lint checks

d2d074b

howard-e requested review from alflennik and jesdaigle May 17, 2021 15:38

howard-e added 2 commits May 17, 2021 11:57

Merge branch 'develop' into import-tests-script-refactor

1d1a4fe

renamed TestPlan.parsedTest.sourceSteps references to `TestPlan.par…

895b987

…sedTest.testActions`

alflennik reviewed May 17, 2021

View reviewed changes

TestPlan.revision is now the latest git tag found; TestPlan.sourceGit…

ffdc802

…Commit renamed to TestPlan.sourceGitCommitHash and adding TestPlan.sourceGitCommitMessage

jesdaigle approved these changes May 18, 2021

View reviewed changes

changed the verbiage in the cli output and comments to reflect a defa…

1be4288

…ult branch

* Deleted TestPlan.revision

bea9364

* Renamed `TestPlan.parsedTest` -> `TestPlan.parsed` * Renamed `TestPlan.parsed.testActions` -> `TestPlan.parsed.tests`

alflennik approved these changes May 18, 2021

View reviewed changes

howard-e merged commit 843afb9 into develop May 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of scripts/import-tests #291

Refactor of scripts/import-tests #291

howard-e commented May 17, 2021 •

edited

Loading

alflennik commented May 17, 2021

howard-e commented May 17, 2021

howard-e commented May 17, 2021

alflennik commented May 17, 2021

alflennik left a comment

alflennik May 17, 2021

howard-e May 17, 2021

alflennik May 17, 2021

s3ththompson May 18, 2021

alflennik May 17, 2021

alflennik May 17, 2021

howard-e May 17, 2021

alflennik May 17, 2021

s3ththompson May 18, 2021 •

edited

Loading

alflennik May 18, 2021

s3ththompson May 18, 2021

alflennik May 18, 2021

alflennik May 17, 2021

alflennik May 17, 2021

jesdaigle May 18, 2021

howard-e May 18, 2021

howard-e May 18, 2021 •

edited

Loading

alflennik May 17, 2021

howard-e May 17, 2021

alflennik May 17, 2021

jesdaigle May 18, 2021

s3ththompson May 18, 2021

alflennik May 18, 2021

s3ththompson commented May 18, 2021

Refactor of scripts/import-tests #291

Refactor of scripts/import-tests #291

Conversation

howard-e commented May 17, 2021 • edited Loading

Changes:

TestPlan.parsed Example

alflennik commented May 17, 2021

howard-e commented May 17, 2021

howard-e commented May 17, 2021

alflennik commented May 17, 2021

alflennik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s3ththompson May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howard-e May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s3ththompson commented May 18, 2021

howard-e commented May 17, 2021 •

edited

Loading

s3ththompson May 18, 2021 •

edited

Loading

howard-e May 18, 2021 •

edited

Loading