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

Install newest Chrome browser before running tests #6115

Merged
merged 12 commits into from
Jan 31, 2020
Merged

Install newest Chrome browser before running tests #6115

merged 12 commits into from
Jan 31, 2020

Conversation

clarmso
Copy link
Contributor

@clarmso clarmso commented Jan 8, 2020

I'm trying to investigate some Chrome 79+ related issues from #6023 by running the existing tests using the latest Chrome browser. We've been lagging in using the latest stable version of Chrome in testing Cypress itself.

I've some thoughts on how to use the latest stable version of Chrome in the testing. This work would enable the Cypress developers to address the compatibilities between Cypress and Chrome in a timely manner.

If it all goes well, we may close #5236. 👍

User facing changelog

Add the ability to test the changes against the latest stable version of the Google Chrome browser.

Additional details

The latest stable version of the Chrome is installed prior to running the test runs that uses Chrome. This way, we don't need to update the Docker image indicated in this line right away when a new version of Chrome becomes available.

- image: cypress/browsers:node12.8.1-chrome78-ff70

Since this Docker image has the repository of Chrome configured, we can just run apt to retrieve the latest version of Chrome.

How has the user experience changed?

There should be no change in the user's experience. The developers can automatically run the tests against the latest stable version of Google Chrome instead of an older one.

PR Tasks

N/A

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 8, 2020

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

  • Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
  • Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

  • The feature/bugfix is self-documenting from within the product.
  • The change provides the end user with a way to fix their problem (no dead ends).

Functionality

  • The code works and performs its intended function with the correct logic.
  • Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
  • The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

  • The code is readable (too many nested 'if's are a bad sign).
  • Names used for variables, methods, etc, clearly describe their function.
  • The code is easy to understood and there are relevant comments explaining.
  • New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
  • There are comments containing link(s) to the addressed issue (in tests and code).

Quality

  • The change does not reimplement code.
  • There's not a module from the ecosystem that should be used instead.
  • There is no redundant or duplicate code.
  • There are no irrelevant comments left in the code.
  • Tests are testing the code’s intended functionality in the best way possible.

Internal

  • The original issue has been tagged with a release in ZenHub.

@clarmso
Copy link
Contributor Author

clarmso commented Jan 8, 2020

I am not authorized to run the server-performance-tests and all server-e2e-tests-* on CircleCI. I cannot test these items on my end. Could someone from Cypress help out?

https://app.circleci.com/github/cypress-io/cypress/pipelines/fa6c0831-9abb-43fb-b6b6-ef56a8322ee5/workflows/b2bed0f6-5601-4953-95b0-ec03ae4dcc19

driver-integration-tests-chrome installs and uses Chrome 79 successfully.
https://app.circleci.com/jobs/github/cypress-io/cypress/227689

@clarmso clarmso changed the title [WIP] Install newest Chrome browser when needed Install newest Chrome browser before running tests Jan 8, 2020
@clarmso clarmso changed the title Install newest Chrome browser before running tests [WIP] Install newest Chrome browser before running tests Jan 23, 2020
@clarmso clarmso changed the title [WIP] Install newest Chrome browser before running tests Install newest Chrome browser before running tests Jan 26, 2020
@clarmso
Copy link
Contributor Author

clarmso commented Jan 27, 2020

Hm...Let me install the latest Chrome browser when it is needed. The installation should occur in the server-e2e-tests-chrome-* and the driver-integration-tests-chrome.

I'm not 100% with the solution because Chrome is (re)installed in every server-e2e-tests-chrome-*. I cannot just cache the .deb file for the Chrome installer because the browser may have other dependencies in the future. "apt-install" is the sure way to install Chrome. In addition, the Chrome installer is fetched from an external server. We really need the connection between CircleCI and the location of the Chrome debian package and its dependencies' to work consistently.

If there's a more efficient solution, please let me or the maintainers know!

circle.yml Outdated
@@ -66,6 +74,10 @@ commands:
steps:
- attach_workspace:
at: ~/
- when:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intended to have the newest Chrome not to be installed for the server-e2e-tests-electron-*. install-latest-chrome is still installed for the electron tests. :( I could add a boolean flag to dictate whether the browser is installed or not. Is there a better way?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you could update the install-latest-chrome command to exit if the supplied browser is not chrome, maybe something like:

if [[ << parameters.browser >> = 'chrome' ]]
then
	echo 'Running Chrome tests, installing latest version...'
else
	echo 'Running in << parameters.browser >>, not updating Chrome...'
	exit 0
fi

circle.yml Outdated
steps:
- run:
command: |
apt update ;
Copy link
Contributor

Choose a reason for hiding this comment

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

apt-get is usually recommended for scripts, but this is probably fine too since we aren't depending on the output

circle.yml Outdated
apt update ;
apt install google-chrome-stable -y ;
which google-chrome ;
google-chrome --version
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add an echo 'Google Chrome version:' above this to help break it out

@clarmso
Copy link
Contributor Author

clarmso commented Jan 30, 2020

I've addressed the issues and the suggestions above. The tests driver-integration-tests-chrome installs the newest stable Chrome successfully, but some tests failed (https://app.circleci.com/jobs/github/cypress-io/cypress/239271). Let me investigate if they're Chrome 79 specific. Those failures should not be related to this change in circleci.yml.

The electron case as seen in server-e2e-tests-electron-* is not tested yet. server-e2e-tests-electron-* looks good to me: Chrome isn't (re)installed.

@clarmso
Copy link
Contributor Author

clarmso commented Jan 31, 2020

The test failure is from packages/driver/integration/commands/actions/type_spec.js with the following error message:

  1) src/cy/commands/actions/type user experience can print table of keys on click:
     TypeError: Cannot convert undefined or null to object
      at Function.keys (<anonymous>)
      at findReactInstance (cypress:///./cypress/support/utils.js:18:20)
      at withMutableReporterState (cypress:///./cypress/support/utils.js:46:24)
      at Context.eval (cypress:///./cypress/integration/commands/actions/type_spec.js:4381:16)

I haven't been able to reproduce this error locally either by running the single test in isolation or all tests from the file. I've Chrome 79.0.3945.130 installed on my computer as well. I don't like intermittent test failures, but let me kick off the tests again through rebasing.

- Add a descriptive name to the install step, so the script doesn't get printed out twice
- Let `browser` be any string - we have more browsers on the way 😄
circle.yml Show resolved Hide resolved
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

Thanks!

@flotwig flotwig merged commit 81555fc into cypress-io:develop Jan 31, 2020
@jennifer-shehane
Copy link
Member

jennifer-shehane commented Feb 5, 2020

Opened a new issue to revert / change implementation of this due to newly exhibited problems. #6328

Note: this implementation was effectively removed as part of https://github.com/cypress-io/cypress/pull/6293/files#diff-29944324a3cbf9f4bd0162dfe3975d88 in order to just get our changes in.

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.

Use newest Chrome (stable) to run tests
3 participants