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

[ccanash]

The objective of this tasks is to research and define what the best protocol will be to communicate "per-test"status updates.

[gnarf]

In order to bring anyone following this issue up to speed, I'm going to post a quick summary of the API in the app for storing statuses / automation jobs (as is).

We have defined a CollectionJobStatus type that has the following values: QUEUED, RUNNING, COMPLETED, CANCELLED, ERROR.

The CollectionJob itself has an externalLogsUrl string state to store additional log file link.

App API: POST /jobs/:jobId/update

Update job status for entire collection job

Parameters:

• status - a CollectionJobStatus
• externalLogsUrl - to update the log url if you have it - only updates when present, empty key is okay

This updates the overall collection job status, this currently isn't displayed anywhere in the app, but is used to clue some calculations based on CANCELLED and QUEUED.

App API: POST /jobs/:jobId/result

Send test results for a specific test in the job

Note: Only works when job status is RUNNING

Parameters:

• testCsvRow or presentationNumber - the v1 or v2 identifier for the test in the job
• responses - an array of strings - one collected response for each action in the test
• capabilities - { atName, atVersion, browserName, browserVersion } to store which browser/at combo has generated results.

This endpoint collects and stores the responses, and checks for "historical responses" match to promote the old assertion values that it can copy.

Displaying the status in the app

BotRunTestStatusList

The data for this is currently coming from this query:

    query TestPlanRunsTestResults($testPlanReportId: ID!) {
        testPlanRuns(testPlanReportId: $testPlanReportId) {
            id
            tester {
                username
            }
            testResults {
                id
                scenarioResults {
                    assertionResults {
                        passed
                        failedReason
                    }
                }
            }
        }
    }

• "Completed" is increased by 1 for every test result existing in the testResults
• "Queued" is the number of totals tests minus the "Completed" count (if job has any status other than CANCELLED
• "Cancelled" ^^^ but when cancelled.

TestNavigator

• "Complete" if scenarioResult has some non-empty output
• "Queued" if overall job is not cancelled
• "Cancelled" otherwise

-- Proposal for updates coming in next message

[gnarf]

Proposal for updates

App/Database Side:

• Create a new database table CollectionJobTestStatus that stores a link to a CollectionJob and a specific Test containing a CollectionJobStatus enum field.
• Upon creating a collection job, create a QUEUED entry in this table for each applicable test in the test plan.
• Update the /jobs/:jobId/update behavior for ERROR and COMPLETED states: any CollectionJobTestStatus for the job that are still QUEUED should be set to CANCELLED
• Update the /jobs/:jobId/results to automatically update the status of a request with results to COMPLETED - long term the automation will likely send "status": "COMPLETED" along with these results, but current deployment does not and this should be a default behavior.
• Update the /jobs/:jobId/results to accept an optional status parameter to update a status for a particular test result - most notably RUNNING and ERROR. Note: Automation side might have some trouble collecting the capabilities used on a failed test, it would be best to not rely on them being present with a status only update.

Frontend

• Update the BotRunTestStatusList component to query a summary of the new table, the count for QUEUED RUNNING COMPLETED ERROR and CANCELLED should all be able to be displayed accurately by summing up the results. Update UI in real time to receive the "per-test” status updates: "started" and "error". #820 has some details for how to display things in the UI
• Update the TestNavigator to query the collection job status per test for bot tests.

Automation side

• Have harness send additional callback to /jobs/:jobId/result with status for RUNNING or ERROR for each test. It might be hard to send capabilities with this, so API side might want to make this needed for requests containing responses. (Currently mocked up in draft PR)

[jugglinmike]

Thanks for taking the time to summarize here, @gnarf. I lost some context over the months since we built the first iteration, so the refresher has been helpful.

Automatic transition from "queued"

• Update the /jobs/:jobId/update behavior for ERROR and COMPLETED states: any CollectionJobTestStatus for the job that are still QUEUED should be set to CANCELLED

I'm on the fence about automatically transitioning to "CANCELLED". I'm tempted to set all the "QUEUED" test statuses to "ERROR" instead. Reason being: reserving "CANCELLED" for explicit human-initiated requests makes it easier to understand how/why tests do not reach the "COMPLETED" state. On the other hand, that approach will implicate individual tests in a way that's misleading (e.g. if the harness crashes during the first test due to an OOM error, it's hardly right to report that there was an error in the eight test).

Taking a step back, this distinction would be moot if the UI surfaced the overall job status--users could recognize what had happened regardless of whether the "automatic test transition" ended up on "ERROR" or "CANCELLED". Do you think we ought to request a refinement on the design of the interface?

Following REST

Given the coordination that deploying this change will require (between ARIA-AT App and the harness), I'm interested in using this as an opportunity to make more severe changes if they make the API more conventional and/or easier to understand. Specifically:

• POST /jobs/:jobId/update to POST /jobs/:jobId, and
• POST /jobs/:jobId/results to POST /jobs/:jobId/test/:testId

The first change is mostly for aesthetics. The semantics of an "update" operation are implied by the HTTP verb "POST", so the word's presence in the URL is redundant.

The second change has a few more practical motivations. First, it drops the term "results", which will be less appropriate when we implement the rest of your proposal (i.e. the route will perform modifications other than setting results). Second, it makes it more clear which conceptual resource is actually being modified. Third, it promotes the required "id" attribute to its traditional position in the URL itself. This isn't just REST for REST's sake--I can imagine situations where being able to see resource IDs in service logs would be helpful.

(Come to think of it, including these more drastic HTTP API improvements might even reduce coordination pressure since ARIA-AT could support both APIs for a transitional period... Though preserving the current semantics might admittedly be more trouble than it's worth.)

What do you think?

[gnarf]

I'm on the fence about automatically transitioning to "CANCELLED". I'm tempted to set all the "QUEUED" test statuses to "ERROR" instead. Reason being: reserving "CANCELLED" for explicit human-initiated requests makes it easier to understand how/why tests do not reach the "COMPLETED" state.

my reasoning for picking cancelled over error was more about not marking tests as "error" when there hadn't been an error in that particular test, just the harness. "Cancelled" seemed to fit the state of the test better in this "one collection job runs all the tests" plan. The "error" status from the job with a "cancelled" test could be rendered as "cancelled due to error" on the frontend as opposed to just "cancelled" (by human hand)

Either way, automatically moving from queued -> cancel/error state I'm happy to implement.

Specifically:

• POST /jobs/:jobId/update to POST /jobs/:jobId, and
• POST /jobs/:jobId/results to POST /jobs/:jobId/test/:testId

I agree with both of these, and think that the backward compatible URLs are possible though because these URLs are provided to the test harness, only need the back compat for POST /jobs/:jobId/test/:testId - something where :testId could be missing and read from testCsvRow or presentationNumber via the POST body. If the eventual goal is to have the callback URL be appended with the test identifier, it would make the callback posting code slightly more complicated on the harness (due to the way we tested it, it's not super simple to just add callbackUrl + testId as we use test urls like http://example.com/?TEST-STATUS=418 for testing.

I only slightly lean toward NOT appending the :testId to the URL to make needing to "compose" the url using a real URL parser unnessecary from the harness POV.

[jugglinmike]

The "error" status from the job with a "cancelled" test could be rendered as "cancelled due to error" on the frontend as opposed to just "cancelled" (by human hand)

Yeah, let's stick with "cancelled."

If the eventual goal is to have the callback URL be appended with the test identifier, it would make the callback posting code slightly more complicated on the harness (due to the way we tested it, it's not super simple to just add callbackUrl + testId as we use test urls like http://example.com/?TEST-STATUS=418 for testing.

I only slightly lean toward NOT appending the :testId to the URL to make needing to "compose" the url using a real URL parser unnessecary from the harness POV.

Given that Node ships with a built-in URL parser, I think this is just a comparison between the presumed utility of more meaningful URLs and the expected inconvenience of testing those URLs. Given that your preference for simpler URLs is weak, I'd like to go with the more expressive design.

[gnarf]

#1000 is a PR for the app-side changes - currently only implementing the backend portions