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.

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

Allow specification of timeout and number of threads in config file #358

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Dec 18, 2015

Wraith uses a standard timeout of 1000 ms and 8 threads. This can cause issues when comparing a large number of pages in that contention is so great the whole page is not retrieved before timing out. This leads to differences being reported that aren't really there.

This PR allows the number or threads and the timeout to use to be specified in the config file, as follows:
num_threads: 3
timeout_ms: 3000

It these values are not specified in the config file, the old defaults of 8 and 1000 are used.

@ChrisBAshton
Copy link
Contributor

Hi @stephen-richards - thanks for this. I'm reluctant to pass even more command line parameters to Phantom/Casper, it's already quite tricky to maintain. Besides, a similar timeout can be achieved using the before_capture hook, e.g.

before_capture: javascript/wait_a_few_seconds.js
// wait_a_few_seconds.js
module.exports = function (phantom, ready) {
    // make Wraith wait a bit longer before taking the screenshot
    setTimeout(ready, 3000); // you MUST call the ready() callback for Wraith to continue
}

(Please note, this example is as of Wraith 3.1, which will happen when #361 is merged).

However, I do like the idea of being able to configure the number of threads Wraith uses! Please consider editing your PR.

expect(wraith.timeout_ms).to eq 1000
end

context 'non-standard config values' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for clarity I'd refer to these as 'overridden' values rather than 'non-standard' (which suggests Strings instead of ints, or non-ASCII characters or something)

@yareckon
Copy link

Please Please Please! My VM is too puny for 8 phantomjs threads, and I would love to be able to set this much lower.

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