Skip to content

System tests with HTML samples.#10553

Merged
feerrenrut merged 4 commits into
masterfrom
addChromeSystemTest
Apr 14, 2020
Merged

System tests with HTML samples.#10553
feerrenrut merged 4 commits into
masterfrom
addChromeSystemTest

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Nov 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Nov 27, 2019

Copy link
Copy Markdown
Contributor Author

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.

Sign in to view
@feerrenrut
Re-write system tests moving logic to Python.
736e2c0 
Adds one basic chrome test.
Explicitly set Python version when running NVDA for system tests
as test runner does not inherit environment variables.
@feerrenrut feerrenrut force-pushed the addChromeSystemTest branch from ef1ca80 to 736e2c0 Compare March 26, 2020 16:48
@AppVeyorBot

This comment has been minimized.

Sign in to view
@AppVeyorBot

This comment has been minimized.

Sign in to view
@feerrenrut
More robust chrome setup
b35a831 
- 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.

Sign in to view
@feerrenrut feerrenrut marked this pull request as ready for review March 27, 2020 09:28
@feerrenrut feerrenrut requested a review from LeonarddeR March 30, 2020 15:56
LeonarddeR
LeonarddeR reviewed Apr 1, 2020
View reviewed changes

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Comment thread tests/system/libraries/ChromeLib.py
Comment thread tests/system/libraries/NvdaLib.py
@feerrenrut

feerrenrut commented Apr 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

feerrenrut commented Apr 3, 2020

Copy link
Copy Markdown
Contributor Author

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

michaelDCurran
michaelDCurran approved these changes Apr 11, 2020
View reviewed changes

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

feerrenrut commented Apr 14, 2020

Copy link
Copy Markdown
Contributor Author

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
@feerrenrut feerrenrut deleted the addChromeSystemTest branch April 14, 2020 14:31
@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
Update changes file for PR #10553
e500d31
@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

Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

+1 more reviewer

@LeonarddeR LeonarddeR LeonarddeR left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature

Projects

None yet

Milestone

2020.2

Development

Successfully merging this pull request may close these issues.

5 participants

@feerrenrut @AppVeyorBot @LeonarddeR @michaelDCurran @nvaccessAuto