Support 0-level assertion priorities on TestRun page #863

howard-e · 2023-12-07T19:59:47Z

How To Run:

yarn db-import-tests:dev;
yarn run dev-debug;

Note 1: This work is dependent on w3c/aria-at#1012 landing and any updated client/resources files here should be reviewed there.

Note 2: This is using a sample example data made available by w3c/aria-at branch collection-0-priority-support-test. You can view the specifics of the change in the description of w3c/aria-at#1014.

Set ariaAtDefaultBranch in import-tests/index.js to collection-0-priority-support-test for testing purposes.

NOTE: REVERT TEST BRANCH BEING IMPORTED IN import-tests/index.js BEFORE MERGING

howard-e · 2023-12-07T20:07:41Z

client/components/common/queries.js

@@ -0,0 +1,183 @@
+import { gql } from '@apollo/client';


For reviewers: I noticed quite some time ago, a LOT of the query params are often repeated. So much so that params are often missed in data update PRs, so I'm proposing to start writing the various individual component queries.js files as a construction of these repeated fragments.

Using this PR as a chance to help visualize that potential change. It's already cut down TestRun/queries.js by a significant amount. If this does cause significant clashes with any other floating PRs (or a proposal to do it differently), I'm happy to revert and make this more of a concentrated task by itself.

I'm down with this. Most fields don't impact query performance so it's totally fine to just query all of them every time. For fields that do impact performance, like tests on TestPlanVersions and testResults on TestPlanRuns, I think it should be divided into two fragments to make accidentally tanking your performance less likely to happen. Something like TEST_PLAN_RUN_FIELDS and TEST_PLAN_RUN_TEST_RESULTS_FIELDS.

I think fields like renderableContent might be preferable to leave off, since they're extremely verbose and "specialty" for specific use cases. If it's there where it's not needed it will definitely make debugging responses harder.

We can manually add them where they're needed.

I can see that benefit, as well as some others, even in the mock testing area. For clarity though, the fields replaced by a fragment has the exact same fields as were listed before, so no additional (or unnecessary) fields just yet. But I do think there's a benefit to this approach, and an even greater benefit if that structure is set up properly.

So I'd rather to remove this proposed enhancement bit from these unrelated code changes and follow up in a separate issue so this enhancement can get it's own focused space.

howard-e · 2023-12-07T20:09:49Z

server/scripts/import-tests/index.js

@@ -53,7 +53,8 @@ Default use:
 const client = new Client();

 const ariaAtRepo = 'https://github.com/w3c/aria-at.git';
-const ariaAtDefaultBranch = 'master';
+// const ariaAtDefaultBranch = 'master';


This needs to be reverted before merging.

Nice trick!

alflennik · 2023-12-12T21:54:28Z

server/graphql-schema.js

+        Indicates if this assertion result needs to be ignored because it is included with
+        a 0-level priority
+        """
+        exclude: Boolean


If some assertions are not relevant, I don't think we should require users of the API to be responsible for detecting that and requiring them to pass in a special assertionResult with an "exclude" flag set to true. It feels like an example where complexity is escaping the abstraction. API consumers should only have to provide assertionResults which are relevant to the testResult.

Addressed in fe222f7

Looks good thanks for addressing that!!

alflennik · 2023-12-12T21:57:20Z

server/graphql-schema.js

+        The same command can be ran during the same test, but in a different
+        mode
+        """
+        settings: String


It is possible to make this an enum?

The values here won't be static. If the support.json is updated with a new AT + settings (or just a new setting in general for a currently imported AT), and the auto-importer triggers, it will fail to query the relevant content if this is an enum of the settings values we know of now.

What do you think about calling this "atOperatingMode"? I have a couple of motivations for the suggestion. One is that since the word "settings" is plural it makes me suspicious that I'm dealing with something like a comma separated list. Another is that settings is quite a broad term, and are not obviously dealing specifically with the AT, for example it could be referring to operating system settings, browser settings, keyboard settings like holding the shift key, any kind of settings, and I think that's one of the things which is confusing me. I suggested "operatingMode" because I feel like "atMode" is a bit too restrictive a term.

Also, can you improve the documentation for this field? I think we should exhaustively list out all the values which are currently used, one by one. Also if you have a link to a specification listing all possible options, putting that here would be hugely helpful as well.

On settings > atOperatingMode request

I suggested "[atO]peratingMode" because I feel like "atMode" is a bit too restrictive a term.

Agreed, I can make that change.

... since the word "settings" is plural it makes me suspicious that I'm dealing with something like a comma separated list.

Note that eventually, this may happen. That's based on the spec request of AT_KEY-commands.csv > settings.

On documentation update request

Also, can you improve the documentation for this field? I think we should exhaustively list out all the values which are currently used, one by one.

Listing them out will require a change any time they're updated, which would be of an unknown frequency. That would make that bit of inline documentation irrelevant for a bit of time and I'm doubtful of how best that will be kept updated.

Also if you have a link to a specification listing all possible options, putting that here would be hugely helpful as well.

I don't but let's suggest updating the v2 Test Format Definition with a section on that and link there. Otherwise, the only spec for that is to check out each key in ats[].settings in support.json, which isn't too easy.

alflennik

Great work! I like what you did with the GraphQL fragments, you were able to delete a surprising amount of code, and that's definitely a good thing.

I feel quite strongly that we should not add a concept of inert assertionResults that API consumers must disable with an exclude flag. If an assertionResult is not relevant I feel like API consumers shouldn't need to provide anything or worry about it at all.

As for an alternate way to do it, one suggestion is that we add the ability to filter test assertions by mode.

It might be more involved to implement up front but I think it will be worth it make sure our abstractions are robust enough to shoulder the weight of all this complexity.

howard-e · 2023-12-13T17:02:28Z

I feel quite strongly that we should not add a concept of inert assertionResults that API consumers must disable with an exclude flag. If an assertionResult is not relevant I feel like API consumers shouldn't need to provide anything or worry about it at all.

@alflennik agreed! I had the same concern when initially making this PR and was hoping to resolve it in a follow up task because of my assumption that further changes to the harness and TestRenderer would be required, which would conflict with already floating changes in that bit of work that would have been too mentally hard to track. Taking another look at it now (and with fresh thoughts 🙂), turns out that wasn't needed. fe222f7 should address this.

alflennik · 2023-12-13T20:29:04Z

Thanks for addressing my big piece of feedback! Also I followed up on the conversation about the settings field.

…0 level assertions will be considered internally as 'EXCLUDE')

…r commands

…aphql-schema.js

alflennik

Thanks for addressing all the changes I requested! Also, big thanks for adding that branch with assertionExceptions populated, that was invaluable to make it easier to test this.

…test format version to also include (an empty) assertionExceptions so hashes will remain in sync; include utility introduced in #880

stalgiag · 2024-02-05T20:52:57Z

server/scripts/import-tests/index.js

+                                        priority = 'SHOULD';
+                                    if (assertionPriority === 3)
+                                        priority = 'MAY';
+


It would be nice to have a utility variable that maps these ints to these strings (which could also be enums). This would help centralize and lift the significance of these relationships which would be helpful for someone like me that was just editing `assertionExceptions`` on my local aria-at without fully understanding what the numbers meant.

Good point. There's the server/resolvers/helpers/convertAssertionPriority.js file, which could be moved to a more appropriate location, and be expanded to something like:

const convertAssertionPriority = priority => { if (priority === 0) return 'EXCLUDE'; if (priority === 'REQUIRED' || priority === 1) return 'MUST'; if (priority === 'OPTIONAL' || priority === 2) return 'SHOULD'; if (priority === 3) return 'MAY'; // ... }; module.exports = convertAssertionPriority;

Would this support what you're thinking of?

That would work. Thank you!

Done in 3d1fbcb

stalgiag · 2024-02-05T21:13:08Z

client/components/TestRun/index.jsx

-                }))
+                assertionResults: assertionResults
+                    // All assertions are always being passed from the TestRenderer results, but in the instance where
+                    // there is a 0-priority exception, and id won't be provided and that cannot be saved


Is this taking advantage of a side effect or is this the strategy used for communicating that an assertion is excluded? It seems easy to misunderstand or forget. I would expect them to be passed prefiltered but I might be missing something.

It's been some time now, so I'll have to look back into why exactly it was done that way. But from what I can remember, the TestRenderer still technically requires the index of that excluded assertion to properly determine which assertions are which, but at some point. It definitely had to be removed before saving and those excluded ones had no ids. That was a part of fulfilling this request with 5c4ec6a.

Is this taking advantage of a side effect or is this the strategy used for communicating that an assertion is excluded?

So seems to be a bit of both.

This point in the code looks like the safest place to filter them out, as the single point before being passed to the server/db. The comment could be much clearer as well.

Oh okay. The background is helpful. I'll leave it to you as to whether you want to change the comment or address it any other way.

Done in af9a5b0

stalgiag

I am approving after our discussion in the two comment threads. I know that some temporary branch settings need to be reset before merging so I will pass this back to you with approval.

jugglinmike

Looks good, @howard-e; this works as expected, so nothing blocking from me.

Note 1: This work is dependent on w3c/aria-at#1012 landing and any updated client/resources files here should be reviewed there.

That patch was recently merged, so I've left my comments here.

jugglinmike · 2024-02-06T00:12:57Z

client/resources/aria-at-test-io-format.mjs

+          let assertionExceptions = json.output_assertions.map(each => each.assertionId);
+          foundCommandInfo.assertionExceptions.split(' ').forEach(each => {
+            let [priority, assertionId] = each.split(':');
+            const index = assertionExceptions.findIndex(each => each === assertionId);


Array.prototype.indexOf would be a bit more readable here

jugglinmike · 2024-02-06T00:13:18Z

client/resources/aria-at-test-io-format.mjs

+            priority = Number(priority);
+            assertionExceptions[index] = priority;


Under the assumption that "fewer mutable bindings is better", these statements could be combined to allow swapping the earlier let for a const.

Sounds good

jugglinmike · 2024-02-06T00:15:10Z

client/resources/aria-at-test-io-format.mjs

+          });
+          // Preserve default priority or update with exception
+          assertionExceptions = assertionExceptions.map((each, index) =>
+            isNaN(each) ? json.output_assertions[index].priority : each


Aren't the members of assertions string values? Returning each from a filter predicate suggests otherwise.

jugglinmike · 2024-02-06T00:17:03Z

server/resolvers/TestPlanRunOperations/createTestResultSkeleton.js

+                        // Filter out assertionResults which were marked with a 0-priority exception
+                        .filter(assertion => {
+                            return !assertion.assertionExceptions?.some(
+                                e =>
+                                    scenario.commands.find(
+                                        c =>
+                                            c.id === e.commandId &&
+                                            c.atOperatingMode === e.settings
+                                    ) && e.priority === 'EXCLUDE'
+                            );
+                        })


The comment doesn't seem to acknowledge the first half of the filter expression, making me wonder whether I understand what's going on here. Would it be accurate to say, "Filter out assertionResults for the current scenario which were marked with a 0-priority exception"?

Stylistically, I think that the closure nesting is reaching a point where it interferes with readability. While the in-line documentation you've suggested helps, breaking this logic out into a standalone function would afford us more room to describe what's going on, reduce the cognitive load (by formalizing the inputs), and limit distraction for folks seeking to understand createTestResultSkeleton at a high level.

/** * Determine whether a given assertion belongs to a given scenario and includes * at least one exception with a given priority. * * @param {string} priority * @param {Scenario} scenario * @param {Assertion} assertion */ const hasExceptionWithPriority = (priority, scenario, assertion) => { return assertion.assertionExceptions?.some( e => scenario.commands.find( c => c.id === e.commandId && c.atOperatingMode === e.settings ) && e.priority === priority ); };

Then invoking like this:

.filter(assertion => !hasExceptionWithPriority('EXCLUDE', scenario, assertion))

I'm only sharing something concrete as a means to demonstrate my point; I'm not too concerned with the specific factoring. For instance, a more purpose-built function would simplify the callsite even further (though it might be difficult to name it).

+1 on this. I was able to take your suggestion and only apply minor changes, thanks! Done in bba8ad4

…possibly have no ids

…ty purposes

howard-e

@jugglinmike I appreciate your feedback on the aria-at related components. Sad that the PR was merged before but I can create a follow up PR there and request your review afterwards

howard-e · 2024-02-06T15:48:43Z

client/resources/aria-at-test-io-format.mjs

+          let assertionExceptions = json.output_assertions.map(each => each.assertionId);
+          foundCommandInfo.assertionExceptions.split(' ').forEach(each => {
+            let [priority, assertionId] = each.split(':');
+            const index = assertionExceptions.findIndex(each => each === assertionId);


howard-e · 2024-02-06T15:51:22Z

client/resources/aria-at-test-io-format.mjs

+            priority = Number(priority);
+            assertionExceptions[index] = priority;


Sounds good

howard-e · 2024-02-06T17:40:39Z

server/resolvers/TestPlanRunOperations/createTestResultSkeleton.js

+                        // Filter out assertionResults which were marked with a 0-priority exception
+                        .filter(assertion => {
+                            return !assertion.assertionExceptions?.some(
+                                e =>
+                                    scenario.commands.find(
+                                        c =>
+                                            c.id === e.commandId &&
+                                            c.atOperatingMode === e.settings
+                                    ) && e.priority === 'EXCLUDE'
+                            );
+                        })


+1 on this. I was able to take your suggestion and only apply minor changes, thanks! Done in bba8ad4

howard-e requested review from alflennik and stalgiag December 7, 2023 19:59

howard-e commented Dec 7, 2023

View reviewed changes

alflennik reviewed Dec 12, 2023

View reviewed changes

alflennik requested changes Dec 12, 2023

View reviewed changes

howard-e mentioned this pull request Dec 14, 2023

Proposal for using Fragments with GraphQL queries #876

Closed

howard-e added 11 commits December 14, 2023 11:46

Update client/resources

69245f8

Update graphql-schema.js and testsResolver

619a5fe

Update assertionResultsResolver.js

0c099f2

Update import-tests script to include assertion.assertionExceptions (…

51ae9a5

…0 level assertions will be considered internally as 'EXCLUDE')

Update getMetrics and queries to be aware of 'excluded' assertions fo…

7c464db

…r commands

Update import branch for testing

bde9219

Update import branch for testing

c7d57fa

Remove 'exclude' property check reliance on frontend

5c4ec6a

Revert graphql fragment usage

7de158b

Fix missing renderableContent in anon viewer query

3cbe58a

Rename instances of Command.settings to Command.atOperatingMode in gr…

635cf65

…aphql-schema.js

howard-e force-pushed the support-0-level-priority branch from 0b2f00a to 635cf65 Compare December 14, 2023 16:46

alflennik approved these changes Dec 14, 2023

View reviewed changes

Paul-Clue mentioned this pull request Dec 19, 2023

Support V2 test format on the test review page #883

Merged

howard-e mentioned this pull request Jan 22, 2024

Update hashTests function to support the migration of test results across test plan versions using the v2 test format #880

Merged

howard-e and others added 5 commits January 22, 2024 13:23

Include migration to update existing test plan versions using the v2 …

318b7f4

…test format version to also include (an empty) assertionExceptions so hashes will remain in sync; include utility introduced in #880

Merge branch 'main' into support-0-level-priority

0d5d165

Move atOperatingMode setting to TestsService, getTests

a26a572

Missing comma

62ecce2

Merge branch 'main' into support-0-level-priority

45d3ed6

stalgiag reviewed Feb 5, 2024

View reviewed changes

stalgiag approved these changes Feb 5, 2024

View reviewed changes

jugglinmike approved these changes Feb 6, 2024

View reviewed changes

howard-e added 4 commits February 6, 2024 09:57

Update comments with reasoning for filtering out assertions that can …

af9a5b0

…possibly have no ids

Update convertAssertionPriority usage

3d1fbcb

Revert test branch being used back to master

942223d

Break out in-line filter into function sufficiently support readabili…

bba8ad4

…ty purposes

howard-e commented Feb 6, 2024

View reviewed changes

howard-e merged commit 8bedf5b into main Feb 6, 2024
2 checks passed

howard-e deleted the support-0-level-priority branch February 6, 2024 20:35

		priority = Number(priority);
		assertionExceptions[index] = priority;

Support 0-level assertion priorities on TestRun page #863

Support 0-level assertion priorities on TestRun page #863

Conversation

howard-e commented Dec 7, 2023 • edited Loading

howard-e Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alflennik Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

howard-e Dec 13, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alflennik left a comment

Choose a reason for hiding this comment

howard-e commented Dec 13, 2023

alflennik commented Dec 13, 2023

alflennik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howard-e Feb 5, 2024 • 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

stalgiag left a comment • edited Loading

Choose a reason for hiding this comment

jugglinmike left a comment • 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 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

howard-e commented Dec 7, 2023 •

edited

Loading

howard-e Dec 7, 2023 •

edited

Loading

alflennik Dec 12, 2023 •

edited

Loading

howard-e Dec 13, 2023 •

edited

Loading

howard-e Feb 5, 2024 •

edited

Loading

stalgiag left a comment •

edited

Loading

jugglinmike left a comment •

edited

Loading