-
Notifications
You must be signed in to change notification settings - Fork 3
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
Callback POST to URL #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks directionally good to me. Beyond the documentation you have in mind, I'd also appreciate some tests.
Although time has proven the entire "agent"/"host" abstraction to be superfluous, I wonder if it's worth honoring here. That is to say: did you consider reporting from the host process? I think it might help with consistency (where the host's responsibilities are limited to collecting data), and it ought to also reduce the amount of plumbing (since we won't have to forward the new parameters across the boundary).
src/agent/at-driver.js
Outdated
this._send({ method: 'session.new', params: { capabilities: {} } }) | ||
this._send({ method: 'session.new', params: { capabilities: {} } }).then( | ||
({ result: { capabilities } }) => { | ||
this.capabilities = capabilities; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's store this as _capabilities
and expose an async getCapabilities
method. That'll increase parity with the WebDriver
class and also make the call site a little cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WebDriver
getCapabilities()
ends up giving back an object that then has get(keyName)
method, would you want me to kind of mirror that same API, or just return the whole object at that point?
async getCapabilities() {
const capabilities = await this.webDriver.getCapabilities();
const browserName = capabilities.get('browserName');
const browserVersion = capabilities.get('browserVersion');
await this.atDriver.ready;
const { atName, atVersion, platformName } = this.atDriver.capabilities;
return { atName, atVersion, browserName, browserVersion, platformName };
}
note the browserName
and browserVersion
= capabilities.get()
calls
The current at-driver we are using (PAC's) does not support getting the capabilities back at any time other than the response to the session.new
, so we still need to store it here on the connection for now.
src/agent/driver-test-runner.js
Outdated
'Content-Type': 'application/json', | ||
}; | ||
if (this.callbackHeader) { | ||
const [name, value] = this.callbackHeader.split(/:\s*/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is intended to hold a secret, it seems possible that the value will include the colon character. However, if we split like this, we'll end up truncating the secret in those cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't 100% sure we wanted to use the header named Secret:
- so put in a single split here, two ways to solve it:
- Hard code the header name (not a terrible option, just want to know for sure what the aria-at-app is going to be looking for)
- Change the split to only match the first
/:\s+/
in the header.
I had not considered posting from the |
Added some snapshot testing of the callback functionality in 39d5ac6 - doing much more than this may require new test harness surface for things (all tests are currently just snapshot wrappers around exec) |
Bringing this one out of "DRAFT" status - would be happy to land as is, or go a little deeper with testing if @jugglinmike thinks it'd be worth the effort. |
src/host/cli-run-plan.js
Outdated
'callback-header': { | ||
describe: 'Header to send with callback request', | ||
default: process.env.ARIA_APP_CALLBACK_HEADER, | ||
// set to hidden if the callback header is set via environment (to not accidentally leak secret in a log) | ||
hidden: Boolean(process.env.ARIA_APP_CALLBACK_HEADER), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/agent/mock-test-runner.js
Outdated
@@ -59,6 +59,7 @@ export class MockTestRunner { | |||
return { | |||
command: command.id, | |||
expectation: assertion.expectation, | |||
output: assertion.expectation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this value distinct so that the tests demonstrate we're not mishandling the expectation
value, e.g. mocked output for assertion "${assertion.expectation}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with the Report from the "requestbin" callback url: {
"method": "POST",
"path": "/",
"query": {},
"client_ip": "13.91.223.73",
"url": "https://eorkylzwxzzltt.m.pipedream.net/",
"headers": {
"host": "eorkylzwxzzltt.m.pipedream.net",
"content-length": "196",
"accept": "*/*",
"accept-encoding": "gzip, deflate, br",
"content-type": "application/json",
"secret": "42",
"user-agent": "node-fetch"
},
"body": {
"testId": 1,
"capabilities": {
"atName": "NVDA",
"atVersion": "2023.2",
"browserName": "chrome",
"browserVersion": "117.0.5938.132",
"platformName": "windows"
},
"responses": [
" alert\n Hello",
" alert\n Hello"
]
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Adds a
--callback-url
and--callback-header
CLI param - causes each test run completion to send an http postTODOs: