Skip to content

Open chrome prior to system tests#14260

Merged
feerrenrut merged 8 commits into
masterfrom
systest-startChromeEarly
Oct 21, 2022
Merged

Open chrome prior to system tests#14260
feerrenrut merged 8 commits into
masterfrom
systest-startChromeEarly

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Oct 17, 2022

Copy link
Copy Markdown
Contributor

Link to issue number:

Splitting up PR #14054

Summary of the issue:

The first system test to run that requires chrome occasionally fails.
This has been replicated on developer machines after chrome updates, however, it is difficult to reproduce, and difficult to detect/recover-from in an automated way.

Description of user facing changes

None

Description of development approach

Theory for cause:
Chrome is busy with post install tasks, so start chrome in the background ahead of the tests.

It may be possible to handle this better within NVDA, however it seems to actually affect users infrequently, and they seem to be able to recover by waiting a few seconds and de/refocus chrome.
In the meantime, the system tests failing delays development.

Testing strategy:

This will require many builds on appveyor to confirm an improvement.

Known issues with pull request:

This is intentionally working around an issue that may affect users.

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
  • Security precautions taken.

@feerrenrut
Open chrome prior to system tests
0756cbb 
Theory: Chrome is busy with post install tasks, so start chrome in the
background ahead of the tests.

The first system test to run that requires chrome occasionally fails.
This has been replicated on developer machines after chrome updates,
however it is difficult to reproduce, and difficult to detect/recover-from
in an automated way.
It may be possible to handle this better within NVDA, however it seems
to actually affect users infrequently, and they seem to be able to
recover by waiting a few seconds and de/refocus chrome.
In the mean time, the system tests failing delays development.
CyrilleB79
CyrilleB79 reviewed Oct 17, 2022
View reviewed changes
Comment thread appveyor/scripts/tests/beforeTests.ps1 Outdated
@feerrenrut feerrenrut requested a review from CyrilleB79 October 19, 2022 08:19
@feerrenrut feerrenrut marked this pull request as ready for review October 19, 2022 08:37
@feerrenrut feerrenrut requested a review from a team as a code owner October 19, 2022 08:37
@feerrenrut feerrenrut requested review from seanbudd and removed request for CyrilleB79 October 19, 2022 08:37
@feerrenrut
fix bad merge
d0fe1c2 
 --lang=en-US was duplicated
@feerrenrut feerrenrut mentioned this pull request Oct 19, 2022
Merged
6 tasks
CyrilleB79
CyrilleB79 reviewed Oct 19, 2022
View reviewed changes
Comment thread appveyor/scripts/tests/beforeTests.ps1 Outdated
# This has been replicated on developer machines after chrome updates,
# however it is difficult to reproduce, and difficult to detect/recover-from
# in an automated way.
# It may be possible to handle this better within NVDA, however it seems to actually affect users

@CyrilleB79 CyrilleB79 Oct 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not understand this specific sentence. Any clarification?

@feerrenrut feerrenrut Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's saying that the bug is legitimate, it has been replicated on developer environments, it is not only theoretical or a quirk of running on Appveyor. Despite it being a real issue, the problem occurs very rarely and working around the issue is very easy, the impact on users is minimal. We won't attempt to fix this given the small impact on users, and high time cost to investigate and fix. However, the issue does affect the system tests, occasionally causing the first chrome test to fail. This is a problem for developers, they have to wait for a new build. This change (starting chrome early) is an attempt to work around the problem, purely to prevent delays for developers.

@feerrenrut feerrenrut Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Clarification:

Despite it being a real issue, the problem occurs very rarely and working around the issue is very easy

Very easy for end users. When I have seen this occur, pressing alt+tab to bring a different app to the foreground, then return to chrome resolves the issue.

@CyrilleB79 CyrilleB79 Oct 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This description is mixing two issues, hence my confusion:

  1. Chrome fails to start normally
  2. First Chrome test fails (due to 1.)

The normal user experience with Chrome and the behaviour of Chrome during system tests are also mixed in this paragraph and sometimes, it's unclear what apply to users and to system tests.

Maybe you could reword the paragraph like this (to be improved and described more precisely):
The first system test to run that requires chrome occasionally fails due to Chrome abnormal startup [please describe more precisely].
Chrome sometimes does not starts normally, preventing users to use it normally. This has been replicated on user machines after chrome updates,
however it is difficult to reproduce, and difficult to detect/recover-from
in an automated way in system tests.
It may be possible to work around this abnormal Chrome startup better within NVDA, however it seems to actually affect users
infrequently, and they seem to be able to recover by waiting a few seconds and de/refocus chrome.
In the mean time, the system tests failing delays development.

Explaining somewhere what happens

@feerrenrut feerrenrut Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

occasionally fails due to Chrome abnormal startup

No, we don't know that for sure. All we know:

  • Occasionally the first chrome system test on Appveyor fails. NVDA does not load a virtual buffer.
  • Very rarely this symptom has been observed by developers on their own machines.

@feerrenrut feerrenrut Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@CyrilleB79 I've updated this explanation.

Comment thread tests/system/libraries/_chromeArgs.py Outdated
Comment thread tests/system/libraries/_chromeArgs.py
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut

feerrenrut commented Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

Dismissing the test failure:

Robot.chromeTests.ARIA treegridRobot4 sec 965 ms
Error message:
Unable to locate 'before sample' marker. See NVDA log for full speech.

This is a symptom of another intermittent problem, an attempt to resolve is coming another PR.

@feerrenrut feerrenrut requested a review from CyrilleB79 October 20, 2022 08:28
CyrilleB79
CyrilleB79 reviewed Oct 20, 2022
View reviewed changes
Comment thread appveyor/scripts/tests/beforeTests.ps1 Outdated
CyrilleB79
CyrilleB79 approved these changes Oct 20, 2022
View reviewed changes

@CyrilleB79 CyrilleB79 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The explanation looks clearer now. Thanks.
Just a typo to fix.

@feerrenrut feerrenrut merged commit cdd19b9 into master Oct 21, 2022
@feerrenrut feerrenrut deleted the systest-startChromeEarly branch October 21, 2022 06:32
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Oct 21, 2022
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

@CyrilleB79 CyrilleB79 CyrilleB79 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component/system-testing

Projects

None yet

Milestone

2023.1

Development

Successfully merging this pull request may close these issues.

5 participants

@feerrenrut @AppVeyorBot @seanbudd @CyrilleB79 @nvaccessAuto