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.

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

Per-test status delivery #47

Merged
merged 8 commits into from
Apr 24, 2024
Merged

Per-test status delivery #47

merged 8 commits into from
Apr 24, 2024

Conversation

gnarf
Copy link
Contributor

@gnarf gnarf commented Mar 21, 2024

No description provided.

@gnarf
Copy link
Contributor Author

gnarf commented Mar 21, 2024

Posting this up as a draft in case @jugglinmike wants to take a look at the approach. I still have a few things to figure out with this one -- testCsvRow doesn't seem to be getting rightly populated, and I'm still pretty sure errors won't be able to "continue" from the agent side, but haven't gotten super far testing it yet.

@gnarf gnarf force-pushed the per-test-status branch from 3cdc2b0 to 8b00f61 Compare March 22, 2024 00:01
src/host/main.js Outdated
Comment on lines 71 to 73
const postCallback = body => {
// ignore if not in callback mode
if (!callbackUrl) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this function is a bit misleading because invoking the function doesn't necessarily cause anything to be posted. I'd support maybePostCallback or keeping the more reasonable name and moving the conditional to each of the call sites.

src/host/main.js Outdated Show resolved Hide resolved
src/host/main.js Outdated
...(callbackHeader || {}),
};
callbackRequests.push(
fetch(callbackUrl, {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code appears to use the same URL for test status updates as it has already been using for AT responses ("results"). Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure what we were going to do for the API on the frontend about it. It would be most convenient if it did use the same URL (no need to pass a second URL to the automation harness), and it does make some sense as these messages are all about a specific test results collection, not the entirety of the job.

My first thoughts were to update the current "results" url to also look for error or status: "TEST_START" and update appropriately.

src/host/main.js Outdated Show resolved Hide resolved
src/host/main.js Outdated Show resolved Hide resolved
@gnarf gnarf changed the title WIP: Per test status updates Per-test status delivery Apr 10, 2024
@gnarf gnarf requested a review from jugglinmike April 10, 2024 20:51
@gnarf
Copy link
Contributor Author

gnarf commented Apr 10, 2024

Updated to support a dynamic :testRowNumber param in the url as per the updates going on over on w3c/aria-at-app#1000

@gnarf gnarf marked this pull request as ready for review April 10, 2024 20:52
@jugglinmike jugglinmike merged commit 1c2f3e8 into main Apr 24, 2024
4 checks passed
@jugglinmike jugglinmike deleted the per-test-status branch April 24, 2024 16:29
@jugglinmike
Copy link
Contributor

Looking good! Thanks, @gnarf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants