Skip to content

System test robustness#13983

Merged
feerrenrut merged 16 commits intobetafrom
systemTestRobustness
Aug 5, 2022
Merged

System test robustness#13983
feerrenrut merged 16 commits intobetafrom
systemTestRobustness

Conversation

@feerrenrut
Copy link
Copy Markdown
Contributor

@feerrenrut feerrenrut commented Aug 4, 2022 •
edited
Loading

Link to issue number:

None

Summary of the issue:

  • System tests (particularly the first to run chrome test) are failing intermittently.
  • This has become an obstruction for making an NVDA release, taking many builds to show that the tests can pass.

Description of user facing changes

For developers:

  • Screenshots on failure are more likely to show something useful. Kiosk mode has been removed.

Description of development approach

Investigated the approach being taken to look for flawed assumptions.

  • Using SetForegroundWindow may be unreliable for the system tests
  • The way that HWND values were retrieved in one case made it easy for mistakes to made when comparing them.
  • Fixed an error when trying to use IAccessible from the incorrect thread.
  • Added further chrome flags disabling behavior we know we don't want during our tests:
  • Clarified "start.exe" process vs chrome process tracking.
  • Tell "start.exe" process to /wait for chrome process to finish before exiting, aiding with tracking of the processes.
  • Added a way to toggle focus in chrome, currently not used.
    • My suspicion is that when NVDA is unable to communicate with Chrome, the issue can be worked around by ensuring chrome looses focus (no longer foreground app), then gains focus again (becomes the foreground app). Having run into this a few times on my own machine.
  • Intentionally toggles focus in chrome to the address bar (alt+d) then to the document with control+F6 chrome shortcut to ensure focus is in the document.
  • Uses 'Report current line in review' (numpad 8) to check if already on the start marker, rather than trying to move away and back.

Testing strategy:

  • Run the tests locally
  • Run many builds on appveyor, looking for similar intermittent failures.

The runs on appveyor:

Known issues with pull request:

None

Change log entries:

None

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@feerrenrut feerrenrut marked this pull request as ready for review August 4, 2022 04:23
@feerrenrut feerrenrut requested a review from a team as a code owner August 4, 2022 04:23
@feerrenrut feerrenrut requested a review from seanbudd August 4, 2022 04:23
seanbudd
seanbudd reviewed Aug 4, 2022
View reviewed changes
@feerrenrut feerrenrut requested a review from seanbudd August 4, 2022 06:13
seanbudd
seanbudd previously approved these changes Aug 4, 2022
View reviewed changes
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
feerrenrut added 15 commits August 4, 2022 15:44
@feerrenrut
Dump speech / devInfo / braille from main thread
972f979 
These may require access to IAccessible or RPC, which must happen from
main thread.
@feerrenrut
Easier test selection for devs
332d64c 
When "--include NVDA" is present, it isn't possible to
select some other tag only, tests matching either tag will be run.

This results in many tests running, when a developer may only wish to run chrome, or symbols tests.

The initial intention was to prevent developers accidentally running installer tests.
This is now achieved by supplying a tag that matches no tests.
@feerrenrut
Ensure hwnd vals are comparible
e34306f 
HWND passed into EnumWindowsProc
was a lpclong, other HWND vals were integer objects,
comparisons for the same HWND were always false.
@feerrenrut
Fix chrome process tracking
fe535fa 
- Keep the 'start' process around until chrome exits (/wait)
  Track this independently of the chrome process
- Look up the chrome window handle and title and save to instance var on
  the library instance. Work with "Window" class instances rather than HWND
  directly.
- Use the start process to confirm exit, logging shows that the start
  process still exists prior to sending control w, but not after.
@feerrenrut
Don't use kiosk mode for chrome
ec34cc9 
So that screenshots can contain more information.
@feerrenrut
Don't use win32 to focus chrome.
300cc50 
The test unable to set the foreground window with user32.SetForegroundWindow
Testing locally shows that this won't work, documentation backs this up.
The RF test runner process is not the foreground process.

See docs: https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-setforegroundwindow#remarks
@feerrenrut
Wait for start marker
041c88f
@feerrenrut
Toggle chrome focus
6a98038 
view desktop
alt+tab back to chrome
wait for chrome to be foreground app
@feerrenrut
Don't exit chrome after each test
7e2b154 
Initially this was done to ensure a "clean slate" for each subsequent
test.
However, each test sample loads in a new tab already, the risk of
interference is low.

On the offchance that closing and opening chrome causes delays, or
background updates to run which may interfere with the tests on
appveyor, this is disabled.

For developers, it is easier to inspect test samples after the test
runs.
This may be important if the test fails.
However, it is noted that it leaves many tabs open for the developer to
tidy up.
@feerrenrut
add note about postmortem debugger
71d9c39
@feerrenrut
Give robot remote server more time to start up
bfa9440 
Appveyor systems seem to have become slower.
Some the amount of time it takes for NVDA to start up is approaching 10
seconds.
@feerrenrut
add logging for the timing of the startup of NVDa
b5c4484
@feerrenrut
review actions
e77548a
@feerrenrut
add docs for toggleFocusChrome
efe6cff
@feerrenrut
fix lint
1b2ef06
@feerrenrut feerrenrut force-pushed the systemTestRobustness branch from 714ae81 to 1b2ef06 Compare August 4, 2022 07:47
@feerrenrut feerrenrut changed the base branch from master to beta August 4, 2022 07:47
@feerrenrut feerrenrut dismissed seanbudd’s stale review August 4, 2022 07:47

The base branch was changed.

@feerrenrut feerrenrut requested a review from seanbudd August 4, 2022 07:48
@feerrenrut
Copy link
Copy Markdown
Contributor Author

feerrenrut commented Aug 4, 2022

These system test changes now target beta. While it is still uncertain about the impact on the system test stability, the risk should be quite low for user functionality.

lukaszgo1
lukaszgo1 reviewed Aug 4, 2022
View reviewed changes
tests/system/libraries/SystemTestSpy/speechSpyGlobalPlugin.py

def dump_braille_to_log(self):
def _dump_braille_to_log(self):
"""Should only be called on main thread"""
Copy link
Copy Markdown
Contributor

@lukaszgo1 lukaszgo1 Aug 4, 2022

Choose a reason for hiding this comment

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

This docstring fails to explain why these methods have to be called from the main thread. Documenting the reasons may allow someone to lift this limitation in the future if this ever becomes a problem.

Copy link
Copy Markdown
Contributor Author

@feerrenrut feerrenrut Dec 8, 2022

Choose a reason for hiding this comment

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

Apparently, this comment was missed. At the very least I'll document it here. Several of these functions rely on NVDA code that must be called on the main thread due to limitations in API's being used.
An example of what happens when they aren't called on the main thread:

DEBUGWARNING - NVDAObjects.IAccessible.IAccessible._get_IAccessibleRole (23:29:19.477) - RF Test Spy Thread (1960): accRole failed: (-2147417842, 'The application called an interface that was marshalled for a different thread.', (None, None, None, 0, None))

I assume this limitation is to ensure thread safety for the API implementation.

@feerrenrut feerrenrut merged commit 4f6ea72 into beta Aug 5, 2022
@feerrenrut feerrenrut deleted the systemTestRobustness branch August 5, 2022 03:12
@nvaccessAuto nvaccessAuto added this to the 2022.4 milestone Aug 5, 2022
@feerrenrut feerrenrut mentioned this pull request Aug 10, 2022
Merged
5 tasks
seanbudd pushed a commit that referenced this pull request Aug 17, 2022
@feerrenrut
Enable rdp via appveyor settings (#14004)
abbf5a6 
Related to #13983

Summary of the issue:
Sometimes the Appveyor environment is unpredictable, recently Docker Desktop opens a window asking for feedback. This window takes the foreground which subsequently results in applications started from the system test being unable to take focus back.

Description of user facing changes
For developers:
The intention is to fix this foreground issue separately, which may take some time. Until then, and for other issues in the future, make it easier to enable RDP as required.

The details of the connection show in the build log at the start of the build (after cloning NVDA)

Description of development approach
When the environment variable for an RDP password is set (via the Appveyor settings), the appveyor RDP script will be run.
NV Access developers will need to set (and unset) this environment variable as required.
@feerrenrut feerrenrut mentioned this pull request Aug 23, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

+1 more reviewer

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component/system-testing

Projects

None yet

Milestone

2022.4

Development

Successfully merging this pull request may close these issues.

5 participants

@feerrenrut @AppVeyorBot @seanbudd @lukaszgo1 @nvaccessAuto