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

Add temporary fix for cucumber tests #4316

Merged
merged 13 commits into from
Sep 4, 2024

Conversation

yoldas
Copy link
Member

@yoldas yoldas commented Sep 3, 2024

Closes #

Changes proposed in this pull request

  • I have applied a workaround for the Sequencescape cucumber tests on CI until the Chrome 128 through Selenium is fixed. I got close to fix almost all the tests but could not get JS running properly with headless=new option and disable-search-engine-choice-screen combination. I used the following info and set it to headless=old to get the tests pass.

SeleniumHQ/selenium#14438

"Driving Chrome 128 through Selenium on a machine without Chrome installed fails connecting to Chrome, be it headless or not, unless asking for headless=old. The disable-search-engine-choice-screen argument does not change anything about this.

It works only if Chrome is directly installed on the machine when not setting --headless=old."

  • The following is explanation of values (new, old, true) passed to headless mode (vs headful):

https://developer.chrome.com/docs/chromium/new-headless

Note that it says the following in that page but it seems to be still supported:

"We intend to remove the old Headless from the Chrome binary and stop supporting this mode"

  • The following is the starting point of my investigation, posted by team members in Slack discussion.

SeleniumHQ/selenium#14453

  • I have also enabled WIP flags for cucumber environment. It is not directly related to the CI issues but I think it is good to have.

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to main]
    - Check story numbers included
    - Check for debug code
    - Check version

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.52%. Comparing base (6b78652) to head (6a9178c).
Report is 14 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4316      +/-   ##
===========================================
+ Coverage    85.51%   87.52%   +2.00%     
===========================================
  Files         1375     1375              
  Lines        29680    29680              
===========================================
+ Hits         25381    25977     +596     
+ Misses        4299     3703     -596     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yoldas yoldas changed the title Add disable-search-engine-choice-screen argument to chrome Add temporary fix for cucumber tests Sep 4, 2024
@yoldas yoldas self-assigned this Sep 4, 2024
Copy link
Contributor

@sdjmchattie sdjmchattie left a comment

Choose a reason for hiding this comment

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

Apart from the WIP flag, well done finding the responsible feature and the flag to work around it.

I don't know if we need the other flags or not but they look sensible. Are there other browsers we can turn to if Chrome does remove this mode?

config/environments/cucumber.rb Show resolved Hide resolved
Copy link
Contributor

@seenanair seenanair left a comment

Choose a reason for hiding this comment

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

Great job 👏

@yoldas yoldas merged commit f8235c1 into develop Sep 4, 2024
23 checks passed
@yoldas yoldas deleted the disable-search-engine-choice-screen branch September 4, 2024 08:29
@yoldas yoldas removed the request for review from StephenHulme September 4, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants