Breaking specs #569

pboling · 2016-11-03T20:36:12Z

Proposed changes in this pull request

This PR is to show what is broken. Some of the things broken are mentioned in the Readme as expected to be working.

Checklist

New tests have been added
~~npm test passes successfully~~
Documentation has been updated

@amir20 to review

…Node vars

pboling · 2016-11-03T20:46:24Z

src/__test__/out_object.spec.js

-    it('#property() works with input params', async () => {
-        let page = await phantom.createPage();
-        let outObj = phantom.createOutObject();
+    describe('#property', () => {


the Property specs here in this describe block pass. All other specs in this change set fail.

amir20 · 2016-11-05T16:10:55Z

src/__test__/out_object.spec.js

-        await page.property('onResourceReceived', function(response, out) {
-            out.lastResponse = response;
-        }, outObj);
+                await page.property('onResourceRequested', true, function(requestData, networkRequest, out) {


@pboling why you passing true to property? That's supposed to be for on only according to the read me.

Oops. This is supposed to be a test of on, not property. Fixed in latest commit.

amir20 · 2016-11-05T16:11:47Z

Almost all of your examples are using page.property('onResourceReceived', true, function(response, out) which isn't right. The README says that's only supported for on. Am I missing something?

amir20

Waiting for changes for property

pboling · 2016-11-06T06:28:36Z

Sorry, that was a transcription error.

The specs that I care most about, because property is not a viable solution for my needs, are the ones with on that pass true. Fixing now.

pboling · 2016-11-06T11:38:19Z

@amir20 the specs have been switched to call on, with true.

pboling · 2016-11-06T11:40:21Z

It is a little strange that I get 9 failures locally and Travis only gets 7, but even still all the ones testing the feature I need are failing on Travis as well as locally.

amir20 · 2016-11-06T18:50:08Z

I see what's going on. The problem is that on and createOutObject have never been tested together. I don't think the README file suggest that it works togther, but I could see how it is confusing. I added property with outobject a long time ago and then someone else added on.

Probably worth doing one of two things:

Update README file to say both don't work. I remember asking in the pull request what is the difference of on with runOnPhantom=true and property . I forget the reason.
Probably deprecate on run with runOnPhantom=true. And fix it so that it works.

amir20 · 2016-11-06T18:53:28Z

src/__test__/out_object.spec.js

-        await page.property('onResourceReceived', function(response, out) {
-            out.lastResponse = response;
-        }, outObj);
+                await page.on('onResourceRequested', true, function(requestData, networkRequest, out) {


await page.property('onResourceRequested', function(requestData, networkRequest, out) { out.lastRequest = requestData; }, outObj);

works. So probably on and outobject have not been tested.

See my other comment

pboling · 2016-11-06T19:46:20Z

@amir20 the second to last line of the Readme section for page#on says the outObject can be used with on, but I agree, it looks like it does not work.

https://github.com/amir20/phantomjs-node#pageon

The same as in property, you can pass additional params to the function in the same way, and even use the object created by #createOutObject().

pboling · 2016-11-06T21:41:03Z

@amir20 I have added robust working examples that accomplish what I need via property to this PR!

pboling · 2016-11-06T21:41:28Z

examples/event.js

+    if (out.isMatch) {
+      responseObj.status = 'aborted';
+      out.urls_aborted.push(responseObj);
+      networkRequest.abort();


networkRequest.abort() does work within property!!!

The thing I was doing that appears to have been breaking it was passing a regex into the outObj. It seems that is not possible. The specs show passing in of simple, JSON-valid data types. Perhaps a Regex is too complex to pass within an outObj?

amir20 · 2016-11-22T16:49:16Z

This PR is no longer valid. Closing. The original issue is still valid and documentation needs to be updated.

pboling · 2016-11-22T18:51:33Z

@amir20 Checking my understanding here:

The failing specs I added are things that are not expected to work, and the documentation just needs to be updated to reflect that.
The passing specs I added are good but tainted in this PR because of the presence of all the failing specs.

If that is correct I will add a new PR that corrects the documentation and also adds the additional specs.

NOTE: I was able to get my code working following the model of the passing specs!

amir20 · 2016-11-22T19:14:10Z

The failing specs I added are things that are not expected to work, and the documentation just needs to be updated to reflect that.

Yes.

The passing specs I added are good but tainted in this PR because of the presence of all the failing specs.

Also yes. I actually didn't know you had added new tests. I'd be more than happy to add those in as a separate PR if you have 'em available. I love more tests. I just couldn't pass through the broken tests.

pboling · 2016-12-02T19:45:19Z

I'll separate out the passing ones in a new PR!

pboling added 3 commits November 4, 2016 02:56

Passing specs for outObject with property('onResourceRequested')

2658eca

six failing specs with #on

77befcc

Breaking specs for #on(onResourceRequested, true) passing outObj and …

855b7f5

…Node vars

pboling commented Nov 3, 2016

View reviewed changes

amir20 reviewed Nov 5, 2016

View reviewed changes

amir20 added Cannot Reproduce On Hold labels Nov 5, 2016

amir20 suggested changes Nov 5, 2016

View reviewed changes

fixed calls to property => on

621f8a3

amir20 added Reproduced Needs Documentation and removed Cannot Reproduce labels Nov 6, 2016

amir20 reviewed Nov 6, 2016

View reviewed changes

Better examples

f1a44e9

pboling commented Nov 6, 2016

View reviewed changes

amir20 closed this Nov 22, 2016

amir20 removed the On Hold label Nov 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breaking specs #569

Breaking specs #569

pboling commented Nov 3, 2016

pboling Nov 3, 2016

amir20 Nov 5, 2016

pboling Nov 6, 2016

amir20 commented Nov 5, 2016

amir20 left a comment

pboling commented Nov 6, 2016 •

edited

Loading

pboling commented Nov 6, 2016

pboling commented Nov 6, 2016

amir20 commented Nov 6, 2016

amir20 Nov 6, 2016

pboling commented Nov 6, 2016 •

edited

Loading

pboling commented Nov 6, 2016

pboling Nov 6, 2016 •

edited

Loading

amir20 commented Nov 22, 2016

pboling commented Nov 22, 2016

amir20 commented Nov 22, 2016

pboling commented Dec 2, 2016 •

edited

Loading

Breaking specs #569

Breaking specs #569

Conversation

pboling commented Nov 3, 2016

Proposed changes in this pull request

Checklist

pboling Nov 3, 2016

Choose a reason for hiding this comment

amir20 Nov 5, 2016

Choose a reason for hiding this comment

pboling Nov 6, 2016

Choose a reason for hiding this comment

amir20 commented Nov 5, 2016

amir20 left a comment

Choose a reason for hiding this comment

pboling commented Nov 6, 2016 • edited Loading

pboling commented Nov 6, 2016

pboling commented Nov 6, 2016

amir20 commented Nov 6, 2016

amir20 Nov 6, 2016

Choose a reason for hiding this comment

pboling commented Nov 6, 2016 • edited Loading

pboling commented Nov 6, 2016

pboling Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

amir20 commented Nov 22, 2016

pboling commented Nov 22, 2016

amir20 commented Nov 22, 2016

pboling commented Dec 2, 2016 • edited Loading

pboling commented Nov 6, 2016 •

edited

Loading

pboling commented Nov 6, 2016 •

edited

Loading

pboling Nov 6, 2016 •

edited

Loading

pboling commented Dec 2, 2016 •

edited

Loading