Many consecutive calls to Run can be collapsed into a single call. While
at it, make the error handling style more consistent. Overall removes 70
lines of repetitive code.
This fixes the data race uncovered by the recent refactor to run all
tests as tabs under the same browser.
The problem was that a write on the pages map could be done from the
goroutine calling NewContext to create a new map, while other goroutines
could similarly read or write the same map.
Instead of adding a lock around the map, make one of the Browser's
goroutines be the sole user of the map. To make that extra obvious and
avoid potential races in the future, declare the map inside the
goroutine's scope.
For some reason, this makes the Attributes tests flakier than before.
For now, add short sleeps; we can investigate that separately, now that
the data races are gone.
The reason that t.Parallel broke the tests was because the parent test
must finish before the parallel subtests can start. So, we'd be closing
the httptest server and removing the tmpfile before any of the parallel
subtests even began.
We could refactor this to make the subtests parallel, but they're only
two, and the parent is already parallel, so it's not worth the effort.
See the added comment.
Remove the log option lines from testAllocate; right now, we don't have
these options for Target, and Target doesn't log much anyway. We can
always revisit this in the future.
While at it, simplify some code.
To fix a potential temporary goroutine leak; see the added comment. The
leaked goroutine would eventually stop once the timer is fired, but
until then, the goroutine could be unnecessarily left around.
This can simplify some common use cases, like running a few actions
directly, or running no actions at all. It's also an almost entirely
backwards compatible change, as all Run call sites should continue to
compile and work.
Leave Tasks, as it can still be useful for functions to return complex
sequences of actions as a single Action.
t.Parallel effectively fires off a goroutine, so we can't use the test
range variable directly. That can result in different subtests using the
same test case data, causing sporadic failures, or some test cases
rarely being actually tested.
This was uncovered while stress-testing the test suite for an unrelated
refactor.
While at it, one test case in TestMouseClickNode was incorrect.
contextmenu fires on a right click, so ModifierNone won't fire it. This
wasn't caught before, as this test case was almost never ran. After the
data race fix, the test case failed consistently, before being fixed.
That way, we avoid the racy map access via Browser.executorForTarget. If
a context is attached to a target, the Target field must be non-nil.
The Browser.pages map is still racy, since multiple tabs can be created
concurrently; we'll fix this other data race in another commit.
This vastly speeds up 'go test' on my laptop from ~10s to ~3s, as we
save a lot of time spinning up new Chrome browser processes.
In practice, each tab is a separate process anyway, but there's a lot of
added overhead if we're firing up the entire browser, particularly with
an empty user data dir.
This makes 'go test' racy now, as Browser doesn't support creating tabs
concurrently right now. Follow-up commits will fix that, with the help
of 'go test -race' after this commit.
We broke this in the refactor because of a nil pointer dereference, but
we didn't catch that as none of the tests loaded a page with an iframe.
That is, a page with multiple frames.
Add such a test, and fix the bug by creating an almost-empty frame when
we start receiving events about a new frame before it's navigated to.
Using a smaller viewport speeds up both tests, and lets us know what
dimensions to expect in TestCaptureScreenshot.
For TestScreenshot, we can know what dimensions to expect in advance, as
we have the images in testdata.
'go test -run Screenshot' goes from ~0.9s to ~0.5s on my machine.
Finally, don't run ExampleTitle as part of 'go test', as it's slow.
The Default folder is created asynchronously to Chrome starting to
listen on the debugging protocol port. So we can't expect it to exist.
Instead, base the example on DevToolsActivePort, which we can rely on.
First, collapse Browser.Start with NewBrowser. There's no reason to
split them up.
Second, unexport Browser.userDataDir, since it's only needed for a test.
It's also a bad precedent, as only the ExecAllocator will control the
user data directory.
Third, export Context.Browser, since we were already exporting
Context.Allocator.
Finally, remove the Executor interface, a duplicate of cdp.Executor.
All it did was wait on the entire allocator, which is confusing. From
the user's perspective, this wait method should instead wait for the
resources for its own browser, and not any other browsers sharing the
same allocator.
We haven't decided how to integrate that into our API, so simply replace
it with Allocator.Wait.
Now that they're both faster and more reliable than before the refactor.
On my laptop, 'go test' now consistently takes ~10s, and I haven't found
any flakes in the past couple of days.
The navigate sleeps can be replaced by appropriate wait actions.
Some other tests don't need any sleeps at all. This might be because
work is done synchronously now; I haven't been able to get test flakes
after hundreds of test runs with flags like -parallel=32 -count=200.
We hadn't noticed a few uncaught exceptions being received from the
browser, because the events were ignored. Start printing them via the
error logger.
The ones we were getting were caused by testAllocate running Navigate
actions when the path argument was empty. Navigating to "testdata/"
causes JS exceptions, as it's not a valid page.
Instead, leave the new target pointing at a blank document.
Use a single websocket connection per browser, removing the need for an
extra websocket connection per target.
This is thanks to the Target.sendMessageToTarget command to send
messages to each target, and the Target.receivedMessageFromTarget event
to receive messages back.
The browser handles activity via a single worker goroutine, and the same
technique is used for each target. This means that commands and events
are dealt with in order, and we can do away with some complexity like
mutexes and extra go statements.
First, we want all of the functionality in a single package; this means
collapsing whatever is useful into the root chromedp package.
The runner package is being replaced by the Allocator interface, with a
default implementation which starts browser processes.
The client package doesn't really have a place in the new design. The
context, allocator, and browser types will handle the connection with
each browser.
Finally, the new API is context-based, hence the addition of context.go.
The tests have been modified to build and run against the new API.
This isn't strictly necessary, as one can always build with the earlier
cdproto version specified in go.mod. However, many people still install
chromedp in GOPATH via 'go get -u', so this workaround makes life easier
for a lot of developers.
Fixes#285.
As spotted in #162 by a contributor, if the context is done before the
Selector.run caller has received from the channel, the spawned goroutine
may leak if blocked on a send.
Turns out that these subtests are the only pair which cannot run in
parallel with each other. Undo that change and add a TODO. This should
fix the CI failures.
While at it, remove an unnecessary testAllocate line.
We know we're going to see an error, so don't log it too:
$ go test -run TestAllocatePortInUse
2019/02/21 17:11:34 ERROR: pool could not allocate runner ...
PASS
ok github.com/chromedp/chromedp 0.004s
The subtests were almost all marked as parallel, but that's not enough.
That only makes the subtests run in parallel with other subtests within
the same tests, not with any other test.
Since none of the tests make use of globals nor require the entire
program to themselves, properly run all the tests in parallel.
Speeds up 'go test' on my 8-core laptop from an average of ~130s to an
average of ~50s. Many tests hit timeouts and have sleeps, so we want to
avoid running those sequentially whenever possible.
It supports alternative names for Chrome such as chromium, as well as
extra names to look for like headless-shell.
Also swap the os.Getenv logic, so that we only do the exec.LookPath work
if the env var is unset.
tip is rather unstable, so we shouldn't block PRs if it happens to break
our build or tests.
While at it, run 'go mod tidy' with the latest tip version.
Before the fix, the added test would give a Pool.Allocate error like:
pool could not connect to 9000: timeout waiting for initial target
The actual underlying error, which can only be seen if one inspects
chrome's stderr, is that it failed to bind to the debugging protocol
port if it was already in use.
This is of course an issue with the environment that chromedp is being
run under, since it was given a port range that wasn't available.
However, the confusing error can lead to developers wasting their time
instead of spotting the error quickly.
Unfortunately, there doesn't seem to be a way to have Chrome exit
immediately if it can't bind to the given port. So, instead of relying
on it, check if the current process can bind to the port first.
Add a test too, where we grab the first port in the pool range, and
check that we get an error that's not confusing.
Fixes#253.
