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

Add bocoup/aria-at as a submodule & implement processing of automated test format #2

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

zcorpan
Copy link

@zcorpan zcorpan commented Dec 2, 2020

See w3c/aria-at#349


Also see w3c/aria-at#350

This works -- the test runs and passes all assertions. Putting it up here for review and discussion. The w3c/aria-at repo should have a way to generate files in this format from the test source material, before adopting this in the main nvaccess/nvda repo.

@zcorpan zcorpan marked this pull request as ready for review December 2, 2020 15:15
Copy link

@rmeritz rmeritz left a comment

Choose a reason for hiding this comment

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

This is all very exciting! Let a few comments that were mostly small readability requests.

Also, the naming conventions here are very inconsistent. There is a mix of python and js style. You probably want to run PEP8. https://pypi.org/project/pep8/ https://pep8.org/#prescriptive-naming-conventions

@@ -46,3 +46,7 @@
[submodule "include/cldr"]
path = include/cldr
url = https://github.com/unicode-org/cldr.git
[submodule "include/w3c-aria-at"]
path = include/w3c-aria-at
url = https://github.com/bocoup/aria-at
Copy link

Choose a reason for hiding this comment

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

This repo has been officially moved to the w3c.

Copy link
Author

Choose a reason for hiding this comment

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

This is intentional, because the PR that adds the JSON test is in a branch of bocoup's fork.

tests/system/libraries/AssertsLib.py Outdated Show resolved Hide resolved
tests/system/libraries/AssertsLib.py Outdated Show resolved Hide resolved
tests/system/libraries/AssertsLib.py Outdated Show resolved Hide resolved
_builtIn.log(obj)
for command, args in obj.items():
if command == 'nav':
# TODO(zcorpan): this should open the file directly, not use iframe.
Copy link

Choose a reason for hiding this comment

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

I'm not sure if you want to be leaving the TODO around.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, for now. Fixing this would need further changes to NVDA's system test code which I didn't want to mess with too much for the prototype.

tests/system/robot/chromeTests.py Outdated Show resolved Hide resolved
tests/system/robot/chromeTests.py Outdated Show resolved Hide resolved
tests/system/robot/chromeTests.py Outdated Show resolved Hide resolved
@zcorpan
Copy link
Author

zcorpan commented Dec 18, 2020

@rmeritz Thanks! I believe I have addressed your comments.

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.

3 participants