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.

Sign up for GitHub

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

Jump to bottom

Switch from PhantomJS to ChromeHeadless for JS tests #33296

Closed
wants to merge 2 commits into from
Closed

Switch from PhantomJS to ChromeHeadless for JS tests #33296

wants to merge 2 commits into from

Conversation

PVince81
Copy link
Contributor

Description

Related Issue

  • Fixes <issue_link>

Motivation and Context

  • PhantomJS is unmaintained and older
  • This will deliver more accurate results and avoid headaches while trying to debug PhantomJS-only test issues as the one in Feature/webdav locking frontend #32250

How Has This Been Tested?

make test-js

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@PVince81 PVince81 added this to the development milestone Oct 25, 2018
@PVince81 PVince81 self-assigned this Oct 25, 2018
@PVince81 PVince81 mentioned this pull request Oct 25, 2018
Merged
12 tasks
@DeepDiver1975
Copy link
Member

conflicting yarn.lock

@PVince81
Copy link
Contributor Author

bah... forgot to pull master

Vincent Petry added 2 commits October 25, 2018 13:23
Switch karma tests to ChromeHeadless
067b826 
This will deliver more accurate results and avoid headaches while trying
to debug PhantomJS-only test issues...
Fix JS test failures due to browser discrepancies
b1fcd2e 
Firefox and Chromium return different values when evaluating
"background-image", absolute URL vs relative URL.

This fix adjusts the TestUtil to normalize the URL to absolute because
the purpose of the test is just to check that the correct target image
was set.
@PVince81
Copy link
Contributor Author

For CI we should look into https://github.com/GoogleChrome/puppeteer/blob/v1.9.0/docs/api.md#environment-variables to avoid redownloading Chrome from mirrors for every run. We should cache this. The PhantomJS packages used to do that automatically.

  • cache chrome download

@PVince81
Copy link
Contributor Author

idea: add chromium to the docker env that runs the JS tests, then set PUPPETEER_SKIP_CHROMIUM_DOWNLOAD

@PVince81
Copy link
Contributor Author

or bundle puppeteer in the global npm in the drone env, hoping that it will only download it once.

will try locally

@PVince81 PVince81 force-pushed the karma-chrome-headless branch from e084085 to b1fcd2e Compare October 25, 2018 11:35
@PVince81
Copy link
Contributor Author

just tried: sudo npm install -g puppeteer fails with permission denied errors, maybe the installation drops back to regular user in some stage.

Also if you set "PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=1 make test-js" it will no use the locally installed chromium. It seems we must download and provide the actual "chrome headless" version somewhere.

@PVince81 PVince81 modified the milestones: development, backlog Oct 31, 2018
@PVince81
Copy link
Contributor Author

not blocking any more, less important => backlog

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 5, 2019

not working as I'd like to and no time to finish

phantomjs is fine for now

@PVince81 PVince81 closed this Feb 5, 2019
@PVince81 PVince81 deleted the karma-chrome-headless branch February 5, 2019 15:24
@lock lock bot locked as resolved and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@felixheidecke felixheidecke Awaiting requested review from felixheidecke

Assignees

@PVince81 PVince81

Labels
2 - Developing
Projects
None yet
Milestone
backlog
Development

Successfully merging this pull request may close these issues.
2 participants
@PVince81 @DeepDiver1975