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

System tests with HTML samples. #10553

Merged
merged 4 commits into from Apr 14, 2020
Merged

System tests with HTML samples. #10553

merged 4 commits into from Apr 14, 2020

Conversation

feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Nov 27, 2019

Example from "Fix double speaking label in angular checkbox" #10552

Summary of the issue:

  • When fixing problems or introducing new features with NVDA working with a browser, we may accidentally introduce regressions. While working here we often develop small samples, but retesting these requires manual work (finding the samples, familiarizing with intent, using the sample correctly) which is prohibitive.
  • System tests written in the Robot Framework language are hard to read with a screen reader, and hard to write due to the reliance on speech output that often has (robot framework) argument separators contained in it.

Description of how this pull request fixes the issue:

  • Introduce automated tests that open Chrome with a specified sample move to the element, assert that the speech is as expected.
  • Use Python for main system test logic. Tests still must have a Robot file, however now this just defines the metadata for the test and calls a single keyword which contains the test logic.
    • Test logic lives in python file with the same name as the robot file.
  • Libraries and test spy code has been re-arranged to make it more clear what is local, what is remote, and what library functions can be called. The convention for Robot Libraries to be in /libraries named SomethingLib the name of the module must match the name of the class used as the library, so the module is given an initial capital letter. Private functions / variables should all start with underscore. Public library "keywords" should use underscore to separate words.
  • Test spy library calls are automatically wrapped with a method so that run_keyword no longer has to be called manually.

Overview of the refactoring done:

  • NVDA Core/startupShutdownNVDA.robot
    • Logic moved to python: robot/startupShutdownNVDA.py
    • Metadata now in robot/startupShutdownNVDA.robot
  • libraries/nvdaRobotLib.py
    • Mostly: libraries/NvdaLib.py
    • Code for installing the Spy and NVDA config. moved to libraries/SystemTestSpy/configManager.py
  • libraries/systemTestUtils.py becomes libraries/SystemTestSpy/blockUntilConditionMet.py.
    • It is shared between Robot framework libraries and the SystemTestSpy package.
  • libraries/helperLib.py code from libraries/AssertsLib.py
  • libraries/sendKey.py becomes libraries/KeyInputLib.py basically a straight rename.
  • libraries/speechSpy.py becomes libraries/SystemTestSpy/speechSpySynthDriver.py
  • libraries/systemTestSpy.py becomes libraries/SystemTestSpy/speechSpyGlobalPlugin.py
  • NVDA Core/variables.py no longer necessary, strings can easily be defined in test code now written in Python

The chrome tests are added:

  • Logic in python: robot/chromeTests.py
  • Metadata in robot/chromeTests.robot

Testing performed:

Ran the tests locally and on Appveyor

Known issues with pull request:

Doesn't close chrome at the completion of the test.

Change log entry:

Section: Changes for developers:

Automated testing of NVDA with Chrome and a HTML sample is now possible.

@feerrenrut feerrenrut requested a review from michaelDCurran Nov 27, 2019
@feerrenrut
Copy link
Member Author

feerrenrut commented Nov 27, 2019

The appveyor docs tell me that Internet Explorer 11, two versions of Chrome, and two versions for Firefox are installed.

@AppVeyorBot

This comment has been minimized.

Adds one basic chrome test.
Explicitly set Python version when running NVDA for system tests
as test runner does not inherit environment variables.
@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

- Ignore separator in application title.
- Allow variations in the number of tab presses required to get to the start marker.
@AppVeyorBot

This comment has been minimized.

@feerrenrut feerrenrut marked this pull request as ready for review Mar 27, 2020
@feerrenrut feerrenrut requested a review from leonardder Mar 30, 2020
Copy link
Collaborator

@leonardder leonardder left a comment

I find this pretty hard to review, at it is not clear to me what's new code and what has been moved somehow. How would you advise to review this?

tests/system/libraries/ChromeLib.py Show resolved Hide resolved
tests/system/libraries/NvdaLib.py Show resolved Hide resolved
@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 1, 2020

I find this pretty hard to review, at it is not clear to me what's new code and what has been moved somehow. How would you advise to review this?

Yeah, I understand. The main thing is getting feedback to know if it's easy to understand how the tests are written. Would you be able to write a system test after looking at this for a bit?

Edit:
Description of refactoring moved to PR description.

I have also updated the description of this PR to point out the conventions used:

The convention for Robot Libraries to be in /libraries named SomethingLib the name of the module must match the name of the class used as the library, so the module is given an initial capital letter. Private functions / variables should all start with underscore. Public library "keywords" should use underscore to separate words.

@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 3, 2020

Since mostly the code has only been re-organized, the most important thing is to ensure that the tests are easy to understand these are the files in tests/system/robot/

Other than that, one interesting change is how to interact with the remote library NvdaSpy (a global plugin that runs a server that the tests connect to in order to extract speech information from NVDA). Previously you had to call run_keyword passing in the function name as a string. Now, these are all converted to methods on the instance. With some type hint trickery you get intellisense (at least in pycharm, hopefully also in vsCode). You can see how this works in libraries/NvdaLib.py _addMethodsToSpy

Copy link
Member

@michaelDCurran michaelDCurran left a comment

I'm happy with the changes, including being able to read the tests themselves. I would like however to see a second chrome test added before this is merged, so we have the confidence that multiple chrome tests don't cause strangeness with Chrome trying to be started twice or what ever would happen.

@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 14, 2020

As we discussed, I have run the same test multiple times (leaving chrome open) which did not cause any issues. I'll go ahead and merge this.

@feerrenrut feerrenrut merged commit 6a2e68a into master Apr 14, 2020
1 check passed
@feerrenrut feerrenrut deleted the addChromeSystemTest branch Apr 14, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.1 milestone Apr 14, 2020
feerrenrut added a commit that referenced this pull request Apr 14, 2020
@feerrenrut feerrenrut modified the milestones: 2020.1, 2020.2 Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants