System test with robot framework #8481

feerrenrut · 2018-07-05T07:07:51Z

Link to issue number:

Summary of the issue:

Introduction of a system testing framework for NVDA.

Description of how this pull request fixes the issue:

Based on the Robot Framework, tests are written in robot files. The tests are discovered and run by the Framework. Each tests sets up an environment for NVDA to run in, starts NVDA as a separate process, and connects to a server running as an NVDA addon. The addon provides access to NVDA internals eg speech commands. The test code then interacts with NVDA as a user would by sending keypresses and checks that the behaviour is as expected using data from the addon.

For more information, see the readme in this PR.

Testing performed:

Run locally against source and installed NVDA, and on appveyor

Known issues with pull request:

Tests currently only look at simple startup/shutdown behaviour.
Developers will need to manually install dependencies via pip in order to be able to run the system tests locally.

Change log entry:

Developers:
A system test framework has been introduced for NVDA. (#708)
A new extension point action for NVDA post startup is now available.

Requires: robotFramework - the system test framework pyautogui - for controlling keyboard/mouse To install: pip install robotframework pip install pyautogui

Ensure this is run from a cmd prompt. Other shells may cause issues, for instance, NVDA does not get correct focus when run from cygwin.

Remote server implemenation and external abstracted library.

Running from tests/system/ did not reduce the number of suites mentioned in Robots reports

LeonarddeR

Congratulations with this huge work. Note that I haven't yet looked into the system testing framework itself, as it looks a bit complex and I'd have to dive into the matter a little more. Some initial remarks though.

LeonarddeR · 2018-07-05T08:35:16Z

appveyor.yml

@@ -109,6 +111,58 @@ build_script:
 - 7z a -tzip -r ..\output\symbols.zip *.dl_ *.ex_ *.pd_
 - cd ..

+before_test:
+ - py -m pip install robotframework
+ - py -m pip install robotremoteserver


Is there a particular reason why this installed twice, also after running submodule update?

Perhaps a mistake, for future reference, the other is on line 65

Ah yes, I remember now. For a little while we were packaging robotRemoteServer with the install of NVDA so that it could be used by the addon. Now we package it within the addon. I will remove this line.

LeonarddeR · 2018-07-05T08:37:03Z

appveyor.yml

+ - py -m pip install robotframework
+ - py -m pip install robotremoteserver
+ - py -m pip install pyautogui
+ - py -m pip install nose


You can also install them in one run, I'd imagine that could be slightly faster.

Yes perhaps so, I'm not sure how much time this will save. It might even make sense to convert this to a requirements file in source control. It would make it easier for developers to ready their own systems to run the system tests.

LeonarddeR · 2018-07-05T08:41:11Z

source/core.py

+import extensionPoints
+
+# inform those who want to know that NVDA has finished starting up.
+postNvdaStartup = extensionPoints.Action()


I believe that, according to earlier convention discussions we had, this should be post_NVDAStartup. Also note that it might make sense to write NVDA in upper case, as in NVDAObjects, NVDAHelper

LeonarddeR · 2018-07-05T08:42:41Z

source/speech.py

+import extensionPoints
+
+# inform those who want to know that there is new speech
+preSpeech = extensionPoints.Action()


Also, this might have to be pre_speech according to the conventions.

I think the conventions were discussed on PR #7606, though the underscores were dropped from that diff. I would be happy with the underscores and certainly want to keep it consistent, perhaps it should be made official on the wiki so that everyone is on the same page. @josephsl would you be on board with this?

what are the performance impacts for this?

There may be a minor performance impact here, very little compared to the rest of what happens before speech goes to the synth.

Which raises a bigger question about the approach of using an extension point to gather the speech, should we be capturing the behaviour that happens between this line, and the eventual getSynth().speak(speechSequence)?

I think we probably should, otherwise there is no way for us to test things like auto language / dialect switching or symbol levels.

Perhaps, instead of an extension point for speech, the addon should install and select a dummy synthesizer.

both could be useful actually. The synth captures the output and the extension point the first call.

LeonarddeR · 2018-07-05T08:51:16Z

tests/system/NVDA Core/variables.py

@@ -0,0 +1,14 @@
+WELCOME_DIALOG_TEXT = (


This might break if a user runs the system tests with a system language other than English, in which the user default language differs.

Yes, it's likely that there will be lots of ways that the tests will not pass on various different systems. To start with, we will be using AppVeyor as the target. Over time, if people are trying to run the tests locally, then the tests will become more tolerant of variations in the underlying system.

Brian1Gaff · 2018-07-05T15:17:36Z

Ah I see so keep on reporting funnies then. Brian

josephsl · 2018-07-05T15:27:07Z

Hi,

I already have tested this branch before this PR was sent.

Regarding variable names: as @LeonarddeR pointed out, there are inconsistencies throughout NVDA as to whether underscores should be included and what not. The best precedence for underscores is event_* definitions, which clearly denotes that the code is part of an event. Also, there are cases where camel case is used for actions, such as postSave method in settings panels.

Personally, I have mixed feelings about it. On one hand, keeping it camel case makes it clear that we want to keep things consistent. adding an underscore may help with readability, but people will still be confused as to what is what. From programming perspective, it doesn't matter as long as the target machine is aware of its states and what it is doing at the moment.

There are other angles to look at. I do know that this is the first time I'm applying rhetorical and communication principles in a pull request, but I think it is time that I mention this due to implications of this change:

I believe that source code is a communicative object. This means people can glimpse the following from source code: mindset, organizational culture, history and direction of a project, dominant messages the project wants to send via code and what not. A well thought-out source code serves to indicate not only the intentions of the developer who wrote it, but also the overall culture and messages an organization has and wishes to spread.

josephsl · 2018-07-05T15:32:35Z

Hi,

Based on what I wrote above, I propose keeping it camel case for now until we get consensus from other stakeholders that we should change this. The chief reason for this is in order to always remember that community is important. Personally, I'll follow the majority opinion if it benefits not only devleopers, but also others.

Thanks.

derekriemer

This is very cool. a few comments.

derekriemer · 2018-07-05T14:45:56Z

appveyor.yml

@@ -20,6 +20,7 @@ environment:

 init:
 - ps: |
+     iex ((new-object net.webclient).DownloadString('https://raw.githubusercontent.com/appveyor/ci/master/scripts/enable-rdp.ps1'))


Having a comment here describing why this is downloaded might help potential future NV access devs if we lose access to the knowledge of why this was originally done.

derekriemer · 2018-07-05T14:55:48Z

appveyor.yml

+     if(!$env:release) {
+      $nvdaLauncherFile+="_snapshot"
+     }
+     $nvdaLauncherFile+="_${env:version}.exe"


a couple of things.

is this compatible with try builds?

What if we offered a python function or something that gave us a launcher file name so we didn't have to do this, and later build the file name in appVeyor? we'd just call py versionBuilder.py, which would import things and use the environment?

Yes, the tests run on try builds, against an install of NVDA. I can't speak to your second question, it might be best for @michaelDCurran to answer that one.

derekriemer · 2018-07-05T16:28:37Z

source/core.py

+import extensionPoints
+
+# inform those who want to know that NVDA has finished starting up.
+postNvdaStartup = extensionPoints.Action()


This should be added to the changes file. This is useful for addons potentially.

derekriemer · 2018-07-05T16:29:58Z

source/speech.py

+import extensionPoints
+
+# inform those who want to know that there is new speech
+preSpeech = extensionPoints.Action()


what are the performance impacts for this?

derekriemer · 2018-07-05T16:33:34Z

tests/sconscript

@@ -20,6 +20,11 @@ unitTests = env.SConscript("unit/sconscript", exports=["env"])
 env.Depends(unitTests, sourceDir)
 env.AlwaysBuild(unitTests)

+systemTests = env.SConscript("system/sconscript", exports=["env"])
+env.Depends(systemTests, sourceDir)
+env.AlwaysBuild(systemTests)


why are these always built?

At the moment, the system tests only run if you explicitly give the option when starting scons. Then we want the tests to run, even if there has been no changes to dependencies.

derekriemer · 2018-07-05T16:50:44Z

tests/system/libraries/nvdaRobotLib.py

+		_pJoin(systemTestSourceDir, "libraries", systemTestSpyAddonName+".py"),
+		_pJoin(profileSysTestSpyPackageStagingDir, "__init__.py")
+	)
+	opSys.copy_file(


what's the difference between this and shutil.copyfile?

Within the system tests I have preferred to use methods available in the system test framework, since they tend to provide more robustness, checks, and reporting when something is not right. This makes it easier to track down the problem in the system test log. Even when things go correctly, there are entries (including timestamps) added to the system test log which can be quite helpful.

derekriemer · 2018-07-05T17:08:27Z

tests/system/libraries/systemTestSpy.py

+import threading
+from systemTestUtils import _blockUntilConditionMet
+from logHandler import log
+from time import clock as _timer


why does this name get changed to _timer instead of _clock

No real reason, except that originally I was importing from timeit import default_timer as _timer

derekriemer · 2018-07-05T17:21:46Z

tests/system/libraries/systemTestSpy.py

+
+	# Private helper methods
+	def _flattenCommandsSeparatingWithNewline(self, commandArray):
+		f = [c for commands in commandArray for newlineJoined in [commands, [u"\n"]] for c in newlineJoined]


that's a work of art.

derekriemer · 2018-07-05T17:23:50Z

tests/system/libraries/systemTestSpy.py

+	def _getJoinedBaseStringsFromCommands(self, speechCommandArray):
+		wsChars = whitespaceMinusSlashN
+		baseStrings = [c.strip(wsChars) for c in speechCommandArray if isinstance(c, basestring)]
+		return ''.join(baseStrings).strip()


Why does this need restripped?

It possibly doesn't need to be there. There is the possibility of a trailing/leading newline character, we want to preserve newline characters within the string. But not at the ends, since they are harder to notice, and can make defining the expected text a frustrating experience.

derekriemer · 2018-07-05T17:29:49Z

tests/system/libraries/systemTestSpy.py

+		return self._spy.getSpeechSinceIndex(speechIndex)
+
+	def reset_all_speech_index(self):
+		self._allSpeechStartIndex = self._spy.getIndexOfLastSpeech()


The name suggests this should reset the speech. Does this actually do anything but return the last index?

Hmm, perhaps it's because its not clear that "all speech" is a grouping. This resets the "all speech" index, meaning that when you next call get_all_speech it will return any new speech added after the index was reset. It's important that we don't actually clear the internal buffer, since tests may need to hold on to speech indexes and be able to refer to them later, without them being invalidated.

feerrenrut · 2018-07-06T02:03:24Z

@josephsl and @LeonarddeR re: underscore vs camel-case

@josephsl I agree with you that source code is a communicative object, and that it's written for people not machines (otherwise we would all be writing in some form of machine code). Code is read many more times than it is written, and as such, being clear is important.

My vote would be on using underscore, because it more clearly defines the pattern. While aiming for consistency across the code base is important, consistency for a paradigm is more so. I couldn't find any opposition to the use of underscores in #7606 where we have already had an extended discussion of this topic. So as the next action, I suggest we all convert our various extension point introducing PR's to use underscores, and I will add a wiki page to document the decision and rationale for the naming scheme.

josephsl · 2018-07-06T02:26:09Z

Hi, I’ll abide by this decision (will modify the PR once the wiki goes up). Thanks.

feerrenrut · 2018-07-06T05:54:13Z

@josephsl https://github.com/nvaccess/nvda/wiki/Contributing#identifier-names has now been edited.

josephsl · 2018-07-06T06:03:57Z

Hi, applying to #7606 now. Thanks.

This captures all NVDA manipulation of speech before it is given to synthesizers.

feerrenrut and others added 30 commits May 16, 2018 08:17

WIP prototype sysTest

f868f5f

Requires: robotFramework - the system test framework pyautogui - for controlling keyboard/mouse To install: pip install robotframework pip install pyautogui

WIP include flaky way of exiting

e9a2af7

Ensure this is run from a cmd prompt. Other shells may cause issues, for instance, NVDA does not get correct focus when run from cygwin.

WIP System testing

1fb27cb

Remote server implemenation and external abstracted library.

Merge branch 'master' into systemTestWithRobot

584ea96

First try at including system tests in appveyor.

5c83124

Disable pyAutoGUI's failsafe check as appeyor moves the mouse???

dd9f3e7

Merge branch 'systemTestWithRobot' into appveyorTests

5a75207

initial.robot: break robot to temporarily bipass freeze.

1e6e265

appveyor.yaml: run robot in powershell so exit code is ignored for now.

69f4407

appveyor.yaml: try again to make system tests return error code 0

9bd9760

appveyor.yaml: try again to make system tests return error code 0

488a33e

appveyor.yaml: fix syntax of uplodFile.

f275871

system tests: tell appVeyor they are junit formatted?

9b889f8

appVeyor: still upload test results on failed builds.

c3b4a90

Upload unitTest reports to appViewer as well.

8b0e1c2

Investigate freeze in robot test.

9270a20

add some logging to nvdaRobotLib

30a1256

More logging

8e02fc4

more logging again

3824887

sleep before connecting to server.

e244bcb

more and more logging

1c4f7b9

Fix logging...

d80c688

reimplement quit nvda test inside initial.robot.

5e130c0

A try at moving everying to robot language to fix freezes.

bdb51da

Launch NVDA from robot with py for appveyor.

1286782

copy the globalPlugin.

a57e95e

Temporarily start rdp for appveyor build

5d540e7

bump

8195ee4

bump 2

6e530b6

bump 3

38e696b

feerrenrut added 2 commits July 4, 2018 13:37

reduce suite verbosity on appveyor

44d94fd

Revert to running from repo root

25f1897

Running from tests/system/ did not reduce the number of suites mentioned in Robots reports

LeonarddeR reviewed Jul 5, 2018

View reviewed changes

feerrenrut added 4 commits July 5, 2018 10:14

Clean up some of the appveyor code

9254b1e

clean up some names and docs for tests

edb0e75

Add scons command to run system tests

7bd629e

ensure dir is created for std out/err logs

2114664

feerrenrut requested a review from michaelDCurran July 5, 2018 12:21

derekriemer reviewed Jul 5, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into systemTestWithRobot

46cfaae

feerrenrut added 4 commits July 6, 2018 08:10

Use dummy speech synth to get speech

f8606bc

This captures all NVDA manipulation of speech before it is given to synthesizers.

wait longer before sending input

c8b9199

Stop update dialog from showing on startup

0e743ac

simplify the appveyor pip installs

21eca3c

derekriemer previously approved these changes Jul 7, 2018

View reviewed changes

michaelDCurran previously approved these changes Jul 17, 2018

View reviewed changes

Update What's new

be440a9

michaelDCurran dismissed stale reviews from derekriemer and themself via be440a9 July 17, 2018 05:48

michaelDCurran approved these changes Jul 17, 2018

View reviewed changes

michaelDCurran merged commit d99bb6a into master Jul 17, 2018

nvaccessAuto added this to the 2018.3 milestone Jul 17, 2018

feerrenrut deleted the systemTestWithRobot branch January 17, 2020 08:59

System test with robot framework #8481

System test with robot framework #8481

Conversation

feerrenrut commented Jul 5, 2018 • edited

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

LeonarddeR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brian1Gaff commented Jul 5, 2018 via email

josephsl commented Jul 5, 2018

josephsl commented Jul 5, 2018

derekriemer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feerrenrut commented Jul 6, 2018

josephsl commented Jul 6, 2018 via email • edited by feerrenrut

feerrenrut commented Jul 6, 2018

josephsl commented Jul 6, 2018 via email • edited by feerrenrut

feerrenrut commented Jul 5, 2018 •

edited

josephsl commented Jul 6, 2018 via email •

edited by feerrenrut

josephsl commented Jul 6, 2018 via email •

edited by feerrenrut