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

Switch Meteor's headless browser tests from PhantomJS to Headless Chrome #9814

Merged
merged 22 commits into from Jul 18, 2018

Conversation

Projects
None yet
3 participants
@toinevk
Contributor

toinevk commented Apr 14, 2018

Per 254, this is an attempt to replace PhantomJS with Headless Chrome.

I evaluated a few different solutions notably:

  • Puppeteer
  • Chrome Launcher + Chrome Remote Interface
  • Selenium

Ultimately, Puppeteer felt the best fit as it maintained by the Chrome team and it
downloads a recent version of Chromium that is guaranteed to work with the API. The only downside is that creates a fresh download (~170Mb Mac, ~282Mb Linux, ~280Mb Win) with every instance of the client. Chrome Launcher will instead find where Chrome is installed on your local file system and set up a debug instance. However, with this method the self-tester would assume that an instance of Chrome is installed that is compatible with the API.

There are some outstanding to dos, but before I proceed thought I would seek some feedback given Im not 100% familiar with Meteor homegrown test runner.

Outstanding To Dos:

  • Add history entry
  • dynamics-imports, modules and app-config tests all use dispatch-mocha-phantom-js.
  • packages/test-in-console - Uses PhantomJS this needs to be removed.

toinevk added some commits Apr 14, 2018

Update Travis
Add stable version of chrome
Update CircleCi
Add config to ensure Chrome runs correctly
Initial Config for Puppeteer
Base config for puppeteer and replace PhantomJS
Fix comment
Make it clear that running with sandbox off is required for
Travis/Circle but is not recommended. This should be further enhanced.
Update Licenses
Found while searching for phantom

@toinevk toinevk force-pushed the toinevk:devel branch from b639134 to ce6945d Apr 15, 2018

Remove PhantomJS from App-Config
Remove PhantomJS from App-Config, Dynamic Import and Modules.
Explicitly end page before browser to ensure both are null.

@toinevk toinevk force-pushed the toinevk:devel branch from 7a53d0f to 4bbc204 Apr 15, 2018

Update CLI Command
Set default to puppeteer

@abernix abernix self-assigned this Apr 18, 2018

@benjamn benjamn added this to the Testing Infrastructure milestone Apr 18, 2018

@abernix

This comment has been minimized.

Member

abernix commented Apr 18, 2018

This looks like a great direction. Thanks for taking the time to work on this!

The one thing we concretely rely on Phantom for, particularly as of Meteor 1.6.2 (soon to be 1.7!), is that it falls into the category of a "legacy browser" (that is to say, it lacks many "modern" browser features like async/await) which allows us to test the new legacy bundle features which 1.6.2 (now, 1.7) touts. (#9439) I still think it's probably worth moving forward on this this change though, and I like the direction and thoroughness you've got going here.

Do you think we could keep tools/tool-testing/clients/phantom/index.js (i.e. the PhantomClient class) in place, but make it so Puppeteer was the default? I think we can just keep the --phantom flag (here for the moment and simply add the --puppeteer flag as an addition. Then we can consider removing it later.

Overall, this PR would be best paired with the work I did in #9364, but try to get over the obstacle I encountered in #9364 (comment). If we got that working, then we could almost confidently remove Phantom, though it might still be worth keeping around for someone wanting to test "legacy browser" features without relying on an "online" service like BrowserStack.

Thanks again for diving into this! (Happy to help more.)

toinevk added some commits Apr 18, 2018

Revert "Update Licenses"
This reverts commit 2bf4425.
Add PhantomJS option back in
Per feedback keep PhantomJS
Replace PhantomJS in Test in Console Runner
Replace Test In Console PhantomJS with Puppeteer. Update Travis node to
current Meteor version for compatibility with Puppeteer.
Add History Entry
Note headless chrome as the default for browser tests.
@toinevk

This comment has been minimized.

Contributor

toinevk commented Apr 19, 2018

Thanks @abernix, I have reverted the change and you can now pass --phantom to run test in the legacy browser. I have also updated the test-in-console runner to use puppeteer instead, note I had to bump the node version in Travis given puppeteer relies on at least version 6.4.0. I have pinned it at the current supported version for Meteor.

There is a single failure with a Blaze test on TravisCI, I dont have a-lot of familiarity here so I will need to unpack that to see if its valid...

@toinevk

This comment has been minimized.

Contributor

toinevk commented Apr 23, 2018

@abernix Is there a reliable way to run the tests in console package locally? I am unable to get it working reliable and the test in question is passing locally. Also given its a Blaze test its turning out tricky to debug! Any tips would be appreciated.

@abernix

This comment has been minimized.

Member

abernix commented Apr 24, 2018

@toinevk Thanks for looking into this.

When I run ./packages/test-in-console/run.sh locally (the command ran here in .travis.yml), I encounter the same failure:

Error

C: tinytest - spacebars-tests - template_tests - javascript scheme urls : !!!!!!!!! FAIL !!!!!!!!!!!

Formatted Output

{
  "groupPath": ["tinytest", "spacebars-tests", "template_tests"],
  "test": "javascript scheme urls",
  "events": [{
    "sequence": 12317,
    "type": "fail",
    "details": {
      "type": "string_equal",
      "expected": "",
      "actual": "http://localhost:4096/"
    },
    "cookie": {
      "name": "spacebars-tests - template_tests - javascript scheme urls",
      "offset": 2,
      "groupPath": ["tinytest", "spacebars-tests", "template_tests"],
      "shortName": "javascript scheme urls"
    }
  }]
}

Which indicates that the error is happening when Puppeteer is running tests via the Chrome Inspector Protocol against its version of Chrome, which uses Chrome 67 (the so-called "Dev" Channel). (See chart of channel status here: https://googlechromelabs.github.io/current-versions/)

Then, instead of running the run-in-console version of the tests, I switched to using the normal "run-in-browser" technique by running:

./meteor test-packages -p 4096 ./packages/non-core/blaze/packages/spacebars-tests

I checked http://localhost:4096/ locally with what was my "Stable" channel (v65, before I restarted — oops, outstanding updates!) and the tests passed. But when I restarted Chrome to get the update, and advanced to v66, the tests started failing.

So basically, I think something may be changing/has changed in Chrome. If you're not experiencing the failure locally, I would try making sure that you've updated your Git submodules for Blaze locally, by running:

git submodule update --init --recursive

(You can check if you were on the wrong commit by going into packages/non-core/blaze and running git status, or by running git -C packages/non-core/blaze status from the root of the meteor checkout). The correct Git hash should be 8f819731df1fce254825c236176cfa5acb2df869.

@toinevk

This comment has been minimized.

Contributor

toinevk commented Apr 25, 2018

Thanks @abernix ! That information was super helpful (my version was also out of date...) - I managed to identify the issue and have submitted a PR at meteor/blaze#284 which goes into the detail. Once this is merged these tests should pass again.

@abernix

This comment has been minimized.

Member

abernix commented Apr 25, 2018

Awesome, thanks for tracking that down @toinevk. That looks like exactly the culprit here.

toinevk added some commits Apr 30, 2018

Retain PhantomJS for Console Tests
Add Phantom back in to retain the ability to run tests in legacy
browser. By default its false.

@toinevk toinevk changed the title from [WIP] Switch Meteor's headless browser tests from PhantomJS to Headless Chrome to Switch Meteor's headless browser tests from PhantomJS to Headless Chrome Jun 6, 2018

@abernix abernix referenced this pull request Jun 14, 2018

Merged

Fix Regressed Test #284

abernix added some commits Jun 14, 2018

@abernix

This comment has been minimized.

Member

abernix commented Jun 14, 2018

@toinevk I've gone ahead and landed meteor/blaze#284 and updated the Blaze submodule within this PRs branch to include the update via my f2a4839 commit above.

It looks like we've solved the failing test, but another (likely legitimate?) failure seems to be occurring, as seen in the TravisCI test run:

(node:5310) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'newPage' of undefined
    at Timeout.runNextUrl [as _onTimeout] (/home/travis/build/meteor/meteor/packages/test-in-console/puppeteerRunner.js:4:30)
    at ontimeout (timers.js:482:11)
    at tryOnTimeout (timers.js:317:5)
    at Timer.listOnTimeout (timers.js:277:5)
process.exit(1);
} else {
await page.close();
setTimeout(runNextUrl, 1000);

This comment has been minimized.

@abernix

abernix Jun 14, 2018

Member

I suspect that this runNextUrl function being called without a browser is the likely cause of the failure identified in #9814 (comment) and shown in this test run.

Fix Regression
Ensure Puppeteer exits successfully when done and no errors have been
found.
@toinevk

This comment has been minimized.

Contributor

toinevk commented Jun 14, 2018

Thanks @abernix, Ill put that down to late night coding... calling nextURL function was a hangover from the way PhantomJS is designed - its not needed for Puppeteer. Travis is passing now so hopefully CircleCI will too and it then should be good to go.

abernix added a commit to meteor/circleci that referenced this pull request Jul 11, 2018

Add new dependencies for Puppeteer.
This relates to the changes introduced in meteor/meteor#9814, particularly
the new Aptitude packages necessary to support the Puppeteer functionality,
as seen in toinevk/meteor@ee47ff5.

By utilizing this CircleCI image that we've already needed to implement for
Android builds, we'll be able to speed up the initial CircleCI start times
by avoiding the need to install the packages from apt sources on every
build.

abernix added some commits Jul 11, 2018

Remove dependency installation which is now bundled into CircleCI Doc…
…ker image.

These are now built into the CircleCI image we already use
(meteor/circleci), thanks to the changes in meteor/circleci#1.
Pass the --phantom flag on all self-test runs, to always test Phantom.
Without the `--phantom` flag passed to the `meteor self-test` command, the
client tests within those self-test test definitions will only be run on
Chrome Headless.

We might want to consider making it the default, so it runs without an
additional argument on local development as well, but this will make sure
that these important Phantom tests run in CircleCI to ensure we're testing
both modern and legacy browsers (of which the later group includes
PhantomJS, in all its "legacy" glory).

cc @hwillson @benjamn for input.
@abernix

This comment has been minimized.

Member

abernix commented Jul 13, 2018

@toinevk I made a few changes above (and via meteor/circleci#1) in order to incorporate the additional packages into our existing CircleCI base image.

I thought we were pretty much ready to merge, but I re-enabled the --phantom flag for all CircleCI runs (with a0578e9), to ensure that we continue to test under both Phantom and Chrome per the above reasons, but it seems that's caused the failures above. Any ideas?

Fix Failing Tests
Ensure Hot Code Push and Package Tests account for possible multiple runs.
@toinevk

This comment has been minimized.

Contributor

toinevk commented Jul 15, 2018

@abernix It took a little while to isolate the issue, but in both instances it was the design of test which didn't account for multiple runs over the same instance of the test application. Given going forward the default locally will be one run and for CircleCI it will be 2, should we document this difference somewhere?

@benjamn

This comment was marked as resolved.

Member

benjamn commented Jul 18, 2018

@abernix If you agree this is ready to go, and you have time tomorrow, could you merge this?

@benjamn benjamn merged commit 50b6751 into meteor:devel Jul 18, 2018

18 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamn

This comment has been minimized.

Member

benjamn commented Jul 18, 2018

Thanks so much for this substantial and valuable work @toinevk!

@abernix

This comment has been minimized.

Member

abernix commented Jul 18, 2018

The final adjustments look great. I'd like to re-iterate the "thanks" that @benjamn has already stated above: Thank you very much, @toinevk!

benjamn added a commit that referenced this pull request Jul 19, 2018

Stop overriding autoupdate version in test-in-browser driver.
This hack dates all the way back to 2013: a2c4a78

Though it is convenient to reload the browser when server files change
while running test-packages, that's not the behavior of most Meteor apps
that use the autoupdate package, and this hack introduced a signficant
difference in behavior between the test-in-browser and test-in-console
driver packages, which finally surfaced due to the interaction between
@toinevk's headless testing PR #9814 and my refactoring of the autoupdate
package (fe9e403). Tests should behave
the same regardless of which driver package is used.

It turns out there's a better way to make the browser reload each time the
server restarts: simply modify Meteor.settings.public, since that object
is included in the client hashes computed by the webapp package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment