Skip to content

Add config.readyEvent and config.delay#167

Open
gentoo90 wants to merge 3 commits intobslatkin:masterfrom
gentoo90:readyEvent
Open

Add config.readyEvent and config.delay#167
gentoo90 wants to merge 3 commits intobslatkin:masterfrom
gentoo90:readyEvent

Conversation

@gentoo90
Copy link
Contributor

If config.readyEvent is set, phantomjs will wait for exactly this console.log message
before taking a screenshot.

config.delay is applied before taling a screenshot (after readyEvent if it's set).

So with config

{
  ...
  "readyEvent": "captureScreen",
  "delay": 1000
}

phantomjs will wait for one second after the string captureScreen is logged to the console.

Both features are from BackstopJS.

Fixes #123.

Add config.readyEvent field.
If set, phantomjs will wait for exactly this console.log message
before taking a screenshot.
Delay is applied after readyEvent (if also applied)
@bslatkin
Copy link
Owner

bslatkin commented Mar 7, 2016

Sorry for my delay. Thanks for taking the time to put this together. The Travis build is broken so I think this patch probably has a bug in it?

Why do you need the page.readyEventOccured = true; part of the patch? I don't understand why using console.log is the best approach. Are you just trying to get around the security boundary?

What I'd do instead is having doInject do a page.evaluate() call to see if an attribute on window is set. If it's not, then go back to waitForReady until the attribute is done. You can define a callback function before the page rendering using page.evaluate() as well. Then use that instead of the console.log you're relying on.

Basically, I'm asking for you to add an API like window.dpxdtReady() within the page and a config option that says to wait for that function. Does that make sense?

@gentoo90
Copy link
Contributor Author

gentoo90 commented Mar 9, 2016

I launched travis build again on the same commit and tests succeeded. The test in pull request failed with

self.assertEquals(work_queue.WorkQueue.DONE, found.status)
AssertionError: 'done' != u'live'

so probably some task from the queue didn't finish in time.

I used console.log to trigger readyEvent because pages may already have some log messages like page loaded and this approach will allow to test them without modifying their code or injecting anything.

@gentoo90
Copy link
Contributor Author

gentoo90 commented Apr 1, 2016

I added window.dpxdtReady field as a possible trigger for page ready event.
But I still would like to be able to use console.log(), from pages own code for example.
@bslatkin could you merge both of this features?

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.

2 participants