Collection Job - Per-test status updates #1000

gnarf · 2024-04-04T19:46:29Z

Implementing the updates for #818

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1206980916128609

alflennik

I mostly reviewed this from the perspective of GraphQL, the database and the general common practices on the backend. On those accounts, it looks great. I also did try to put on an automation hat and I was able to successfully get "QUEUED", "COMPLETED", "CANCELLED" statuses, but I would appreciate if the other reviewers could give the automation side a good thorough test. I didn't see any unexpected behavior.

It will be great to hugely reduce the number of server requests soon!

alflennik · 2024-04-11T19:40:59Z

server/models/CollectionJob.js

+            as: 'testStatus',
+            foreignKey: 'collectionJobId',
+            sourceKey: 'id'
+        });


Do we want the collection job to be deleted when the test plan run is deleted?

I debated this myself - we have some recovery potential via the database if not, but we might want to delete them

alflennik · 2024-04-11T19:41:47Z

server/migrations/20240404171101-addCollectionJobTestStatus.js

+                        type: Sequelize.INTEGER,
+                        allowNull: false,
+                        onDelete: 'CASCADE',
+                        onUpdate: 'CASCADE',


Interesting feature! Definitely going to file this trick away in my brain.

jugglinmike

I'd like to formalize the distinction between "new way" and "old way" a bit. Doing so will further clarify the differences, give us more confidence when removing the deprecated logic in the future, and reduce the number of unnecessary code paths in the interim.

Basically, if we start with something like,

const isEnhanced = 'testRowNumber' in req.params;

Then we could switch on isEnhanced. (That name could probably be improved, though.)

jugglinmike · 2024-04-24T19:20:51Z

server/controllers/AutomationController.js

+    // When new status is 'COMPLETED' or 'ERROR' or 'CANCELLED'
+    // update any CollectionJobTestStatus children still 'QUEUED' to be 'CANCELLED'
+    if (
+        status === COLLECTION_JOB_STATUS.COMPLETED ||
+        status === COLLECTION_JOB_STATUS.CANCELLED ||
+        status === COLLECTION_JOB_STATUS.ERROR
+    ) {


A utility function like isFinal (maybe defined as a static method on the new model) would surface the intent and make this more readable.

gnarf · 2024-04-25T14:51:16Z

I'd like to formalize the distinction between "new way" and "old way" a bit. Doing so will further clarify the differences, give us more confidence when removing the deprecated logic in the future, and reduce the number of unnecessary code paths in the interim.

We've landed the automation-harness patch so it should be safe to remove the deprecated paths entirely, would you prefer I do just that?

jugglinmike · 2024-04-25T20:04:02Z

I think that could work. Don't we also have to update the callback URLs?

gnarf · 2024-05-01T15:08:12Z

I realized while updating the PR to remove the back-compat URLs that some mock work I needed had already been done on the working UI branch so I merged that into this one instead of keeping them separate. I could tease the frontend updates out of this branch if people still prefer. The changes from d87280b adds the frontend for the test queue and manage nvda run dialogs and some ways to test other status.

gnarf · 2024-05-01T18:41:12Z

Alright - front end tests are now passing as well, so this should be all good - I removed the back-compat URLs as well because the automation harness is properly working with the new style URLs already.

alflennik

So this is a huge improvement vastly reducing the load on the server and preventing issues we've already seen that took down the server.

I don't see any reason not to get this in, it looks great and I see a lot of small improvements sprinkled here and there which is great.

I verified that it works as intended with both the mock and genuine automation systems.

I feel like in a future update it might be nice to improve a few quirks of the system as it stands now. For example I noticed it doesn't check for status updates on the test run page. This is definitely an improvement we can consider separately and not a blocker.

Secondly, there are still multiple requests happening every second (I see a "TestPlanRunAssertionResultsByTestPlanRunId" query as well as the "TestPlanRunsTestResults" query) and I feel like the architecture of GraphQL should theoretically allow them to all be combined into one. Again, not a blocker.

The main thing which is worth looking at down the line, but not a blocker, is that fact that the system still makes one request for every active collection job in progress. It feels like this will only be an issue if the admins are trying to test every dang test plan that exists, all at once, and while that doesn't seem likely to happen anytime soon, it is pretty conceivable that it could start to happen eventually. That would be a nice enhancement, but this PR solves the much more serious issue of making one request per test.

Again these notes really have nothing to do with the PR itself, which is all about "per test status updates." So I'm definitely happy to approve.

This ended up being a pretty large-scale change, which was not the expectation going into it, so nice job chasing it down through the whole stack and seemingly every system we got 🙏

alflennik · 2024-05-06T17:23:14Z

client/components/BotRunTestStatusList/index.js

-const BotRunTestStatusList = ({ testPlanReportId, runnableTestsLength }) => {
+const pollInterval = 2000;
+
+const BotRunTestStatusList = ({ testPlanReportId }) => {
    const {
        data: testPlanRunsQueryResult,
        startPolling,
        stopPolling


Wow I had no idea these existed, nice use of the tech 😮

alflennik · 2024-05-06T17:26:44Z

server/tests/integration/graphql.test.js

+                    transaction: false
+                }
+            )
+        ).rejects.toBeDefined();


Thanks for covering this less obvious gotcha in GraphQL, I appreciate that!

alflennik · 2024-05-06T17:27:26Z

server/tests/util/mock-automation-scheduler-server.js

@@ -12,6 +12,9 @@ const {
 } = require('../../middleware/transactionMiddleware');
 const { query } = require('../util/graphql-test-utilities');

+// 0 = no chance of test errors, 1 = always errors
+const TEST_ERROR_CHANCE = 0;


jugglinmike

Way to go, @gnarf!

Issues addressed: * #1105, addresses w3c/aria-at#1070 * #1053, addresses w3c/aria-practices#2971 * #1097, addresses #977 * #1095, addresses #991 * #1093, addresses #934 * #1000, addresses #818 * #1089, addresses #992 * #1067, addresses #993 * #1056, addresses w3c/wai-aria-practices#212 --------- Co-authored-by: alflennik <alflennik@users.noreply.github.com> Co-authored-by: Paul Clue <67766160+Paul-Clue@users.noreply.github.com> Co-authored-by: Mx Corey Frang <corey@bocoup.com> Co-authored-by: Mx. Corey Frang <gnarf37@gmail.com> Co-authored-by: Erika Miguel <erika@bocoup.com> Co-authored-by: Mike Pennisi <mike@mikepennisi.com>

gnarf mentioned this pull request Apr 4, 2024

Automation: Research protocol for communicating “per-test” status updates #818

Open

howard-e changed the base branch from main to development April 4, 2024 22:27

gnarf force-pushed the per-test-status branch from 2fe3b05 to fabbf88 Compare April 10, 2024 17:35

Per-Test status updates - basic queries and endpoints

95a2bb8

gnarf force-pushed the per-test-status branch from fabbf88 to 95a2bb8 Compare April 10, 2024 17:46

gnarf added 4 commits April 10, 2024 14:50

updates for tests - new urls and checking testStatus

f5ea065

Add test for test-status-only update

79d0b73

more testing for per-test-status crud

0e16b81

add tests for old urls/format

d699e84

gnarf requested a review from alflennik April 10, 2024 20:21

gnarf marked this pull request as ready for review April 10, 2024 20:21

gnarf requested a review from jugglinmike April 10, 2024 20:21

gnarf mentioned this pull request Apr 10, 2024

Per-test status delivery w3c/aria-at-automation-harness#47

Merged

alflennik approved these changes Apr 11, 2024

View reviewed changes

gnarf added 2 commits April 24, 2024 10:38

Working queries and updates to get BotRunTestStatusList to work

e945cb9

more updates for test queue page view

6d1d185

gnarf mentioned this pull request Apr 24, 2024

per-test status updates frontend and mock helpers #1058

Merged

gnarf added 2 commits April 24, 2024 12:11

update tests to pass

b2c6927

ensure recursive query fails

ff36385

jugglinmike requested changes Apr 24, 2024

View reviewed changes

gnarf added 3 commits May 1, 2024 10:50

update to new urls

c3f4d74

Remove back-compat URLs

cf50111

Merge branch 'per-test-status-2' into per-test-status

d87280b

gnarf added 3 commits May 1, 2024 11:40

testing tests on github

292527d

remaining tests

6d4ce9c

correct test mock

497ae8c

gnarf requested review from jugglinmike and alflennik May 2, 2024 01:14

alflennik approved these changes May 6, 2024

View reviewed changes

jugglinmike approved these changes May 8, 2024

View reviewed changes

jugglinmike merged commit 4a2ad52 into development May 8, 2024
2 checks passed

jugglinmike deleted the per-test-status branch May 8, 2024 23:20

howard-e mentioned this pull request May 28, 2024

May 28, 2024 Release #1112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection Job - Per-test status updates #1000

Collection Job - Per-test status updates #1000

gnarf commented Apr 4, 2024 •

edited

Loading

alflennik left a comment

alflennik Apr 11, 2024

gnarf Apr 11, 2024

alflennik Apr 11, 2024

jugglinmike left a comment

jugglinmike Apr 24, 2024

gnarf commented Apr 25, 2024

jugglinmike commented Apr 25, 2024

gnarf commented May 1, 2024

gnarf commented May 1, 2024

alflennik left a comment

alflennik May 6, 2024

alflennik May 6, 2024

alflennik May 6, 2024

jugglinmike left a comment

Collection Job - Per-test status updates #1000

Collection Job - Per-test status updates #1000

Conversation

gnarf commented Apr 4, 2024 • edited Loading

alflennik left a comment

Choose a reason for hiding this comment

alflennik Apr 11, 2024

Choose a reason for hiding this comment

gnarf Apr 11, 2024

Choose a reason for hiding this comment

alflennik Apr 11, 2024

Choose a reason for hiding this comment

jugglinmike left a comment

Choose a reason for hiding this comment

jugglinmike Apr 24, 2024

Choose a reason for hiding this comment

gnarf commented Apr 25, 2024

jugglinmike commented Apr 25, 2024

gnarf commented May 1, 2024

gnarf commented May 1, 2024

alflennik left a comment

Choose a reason for hiding this comment

alflennik May 6, 2024

Choose a reason for hiding this comment

alflennik May 6, 2024

Choose a reason for hiding this comment

alflennik May 6, 2024

Choose a reason for hiding this comment

jugglinmike left a comment

Choose a reason for hiding this comment

gnarf commented Apr 4, 2024 •

edited

Loading