Skip to content
This repository has been archived by the owner on Jun 19, 2020. It is now read-only.

Support for phantomjs 2.0.0 and 2.0.1 pre-release with fileupload bugfix #1352

Merged
merged 89 commits into from Dec 1, 2015
Merged

Support for phantomjs 2.0.0 and 2.0.1 pre-release with fileupload bugfix #1352

merged 89 commits into from Dec 1, 2015

Conversation

istr
Copy link
Collaborator

@istr istr commented Nov 22, 2015

This PR contains:

  • the changes contained in Test suite adapted to test both slimerjs and phantomjs #1351 to make tests pass for slimerjs
  • lots of adaptations to the test suites to cater for changes in phantomjs 2.0.0 output (stack traces and syntax errors)
  • a fix to the patchRequire logic such that it will be applied to phantomjs 2

This closes / supersedes #1310, #1137, #1173, #1291, #1189, #1351, https://github.com/n1k0/casperjs/issues/1204, #775.

hexid and others added 30 commits August 21, 2015 11:30
Removed patchRequire() as it is no longer needed for 2.x
Removed coffeescript tests as v2.x removed support
Removed custom bind implementation as v2.x implements this
Switched from phantom.args to system.args as v2.x deprecated the former
isJsFile now checks require.extensions (minus json) to determine valid
javascript extensions
Modified tests so that they accomodate engines (PhantomJSv2) that don't
support coffeescript
system is no longer exposed as a global variable
phantom.args has been deprecated with PhantomJS v2.x
system.args is available with all supported engine versions
Version check now checks based on the engine in use
This will allow SlimerJS to work when it reaches v1.0.0
Add `CASPERJS_ENGINE` environment variable
 * The `--engine=` flag takes priority over this
Add `ENGINE_EXECUTABLE` environment variable
 * Engine specific env vars take priority over this
Add PhantomJS version 2.0.0
Add SlimerJS version 0.9.6
Fix new engine URLs (Phantom v2.0.0 and Slimer v0.9.6)
Launchers now utilize the `ENGINE_FLAGS` env variable
Travis no lnger appends `ENGINE_FLAGS` to `ENGINE_EXECUTABLE`
ENGINE_EXECUTABLE should now be used by make
This will most likely cause other tests to fail
Travis treats each element as one line, so semicolons are needed
Added `event` as an argument to `window.oncontextmenu`
This works in Phantom because Webkit assumes an `event` var by default.
Running `window.oncontextmenu = function() { event.preventDefault(); }`
in Chrome prevents opening the context menu on a page, where Firefox
would throw the error: `event is not defined`.

Fixed some formatting
Changing from 'name' to 'username' variable fixed this.
window.name defaults to an empty string in Firefox and it seems to be
reset by the time Casper is able to access it in an `evaluate` call.
@n1k0
Copy link
Member

n1k0 commented Nov 25, 2015

Here is my proposal for an engine specific test skip function in modules/tester.js

You could also user a min, maxVersion and utils.cmpVersion; sample api:

test.skipIfEngine(nb, {name: "phantomjs", {min: "*", max: "1.9.0"}})

@istr
Copy link
Collaborator Author

istr commented Nov 25, 2015

Hopefully addressed all changes @n1k0 mentioned.

Introduced test.skipIfEngine and utils.matchEngine. Tests for this new functionality are still missing but I would like to address those in a subsequent feature branch to get 2.0.0 support landed.

(Edit: ...and at least issue a 1.1-beta4 that contains 2.0.0 support to allow for broader testing.)

Please review (build passes).

@paazmaya
Copy link
Contributor

@istr would you consider possible that CasperJS v1.0.x is supporting only up to PhantomJS v.1.9.x, and CasperJS v2.0.0 starts the support for for PhantomJS >=v2.0.1 (while also keeping v1.9.x supported)?
See discussion #1177

@istr
Copy link
Collaborator Author

istr commented Nov 26, 2015

@paazmaya To be honest I don't really care much about versioning policy. From what I gather in the old issues marked milestone 2.0.0 and marked milestone 1.1 and from the discussion threads I read, the current setup is as follows:

  • 2.0 marked issues may contain substantial changes to the API and might break BC
  • 1.1 marked issues do not contain any changes that break 1.0 / current master API and/or basic behavior, try too keep BC as much as possible, but affect support of the underlying engines

That is from my understanding:

  • 1.0.* supports phantomjs up to 1.8.* (< 1.9.0) and is marked obsolete / stable
  • 1.1.* supports phantomjs 1.9.1 and later as well as slimerjs (experimental)
  • 2.0.* will support phantomjs 2 and later and possibly breaks API with 1.* versions

In short: I don't see that the CasperJS jump from major 1.* to major 2.* has to be coupled to phantomjs2 support, but should reflect a major change within CasperJS itself (which phantomjs2 support definitely isn't).

So to not slow down things I would prefer to proceed along these lines:

  • get 1.1 ready (after some two or more beta cycles for bugfixing only) with complete BC and as much engine support we could currently get (this PR)
  • schedule, sort and discuss 2.0 feature set (with possible API changes and possibly breaking BC) thereafter

The only thing that matters for me (and possibly most others waiting for phantomjs2 support) is to get out a next publicly available beta ready for testing as fast as possible.

@paazmaya
Copy link
Contributor

@istr very well argumented. Let's make 1.1!

@istr
Copy link
Collaborator Author

istr commented Dec 1, 2015

@paazmaya @hexid Any chance you could do a last review and merge this PR? Even though I could do it on my own now, I would dislike to merge in this large change-set without approval ("four-eyes principle").

@paazmaya
Copy link
Contributor

paazmaya commented Dec 1, 2015

@hexid may you do the honour?

@hexid
Copy link
Collaborator

hexid commented Dec 1, 2015

It looks like everything @n1k0 mentioned was fixed.
I'm busy at the moment, but I should be able to look it over in about an hour.

@hexid
Copy link
Collaborator

hexid commented Dec 1, 2015

Everything looks good to me.

I couldn't find an issue that documents that the SlimerJS clitests are being skipped.
I'll go ahead and create one and then merge this PR.

@hexid
Copy link
Collaborator

hexid commented Dec 1, 2015

Merging

hexid added a commit that referenced this pull request Dec 1, 2015
Support for phantomjs 2.0.0 and 2.0.1 pre-release with fileupload bugfix
@hexid hexid merged commit 340e028 into casperjs:master Dec 1, 2015
@davidosomething
Copy link

👏 🎆

@paazmaya
Copy link
Contributor

paazmaya commented Dec 1, 2015

🍇 and 🍷 !

@istr
Copy link
Collaborator Author

istr commented Dec 1, 2015

@hexid Nice, thanks a lot! Will close the issues PRs that are superseded now. Let's see how to proceed with the next beta and what open 1.1 issues to include.

@istr istr deleted the feature/phantomjs2 branch December 1, 2015 17:01
@n1k0
Copy link
Member

n1k0 commented Dec 1, 2015

Thanks to everybody involved in achieving this and for the huge help provided. The project owes you many beers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants