-
Notifications
You must be signed in to change notification settings - Fork 15
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
Feat: Enable automation support for VoiceOver/Safari/MacOS #1101
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.
Confirmed this allows VO and Safari tests to be queued up and ran as expected. Great stuff!
I left a minor question inline about a logging
variable.
However, the main reason I think a 2nd look should be had here is because the prompt to trigger automation when an eligible report is created seems to be missing. I would have expected to be able to simply select that when creating a VoiceOver
report now. In fact, it is also missing for when I'm creating an NVDA report as well. I confirmed it still popped up on development
as shown in the screenshot but didn't on this branch.
@@ -31,6 +31,7 @@ export default ( | |||
ats: [ | |||
{ | |||
id: '1', | |||
key: 'jaws', |
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.
In an effort to limit the amount of files changed per PR whenever a mock is involved, may be worth acknowledging that a lot of the mock data used throughout is structurally very similar and also use the same data in a lot of instances.
Thinking using a construction of "pieces" of mock data may be helpful so only a single location has to change to make it easier for future reviews.
This can be discussed outside of this PR of course and not expecting any change here.
@@ -71,7 +71,7 @@ const getById = async ( | |||
*/ | |||
const getByQuery = async ( | |||
model, | |||
{ where, attributes = [], include = [], transaction } | |||
{ where, attributes = [], include = [], logging, transaction } |
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 don't see logging
being passed in any calls. How is logging
being used?
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.
it's not, but it's clever - if you having trouble figuring out why the SQL query isn't working passing logging: console.log
will dump the build SQL query - I can remove it but I wanted to point it out - glad you caught it
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.
Yep you could remove it, but very helpful to note down!
Nice catch! I can take a quick look, but it's likely that the test report isn't passing the |
@gnarf sure! On
|
a659c83 should fix the issue with the add and run popup: |
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.
Changes look good!
Although the test failure is quite puzzling to me (because I can't get it to fail locally). It may be worth figuring out but the assertion will have to be updated to 1
anyways because of #1097 changes recently being included in development
that do away with the <span class=sr-only>
.
The test that was failing was on the merged branch, so it made sense I just needed to update for the new assumptions. |
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.
Thanks for addressing the feedback!
No description provided.