fix(browser): ensure browser state is EXECUTING when tests start

[solo-md]

Browser state is EXECUTING when it actually started to execute tests. This state change is triggered by client on actual tests execution start.

Fixes #1640

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[solo-md]

I signed it!

[googlebot]

CLAs look good, thanks!

[johnjbarton]

Sorry the vacation delay....

[johnjbarton]

Changing this.state in client callbacks responding to events makes sense: we should to this uniformly unless there is some oddity which prevents us. Therefore please try to remove the this.state = EXECUTING in the client command execute(). The execute() command should not set the state; it should be set as you do here, in the start callback.

[solo-md]

I agree that execute() command shouldn't set state to EXECUTING, as actually tests are not executing at that moment and before onStart(). But we should make a difference between READY state of the browser and the state which is after execute() and before onStart(). We want at least the browser's methods onKarmaError() and onInfo()during this state.

I've added additional state called CONFIGURING, which explains the actual state between execute() and onStart(). Is that OK?

[johnjbarton]

Yes! This is better. How about one more improvement? The READY state is IMO misnamed. After this change the states are:

Browser.STATE_READY = READY
Browser.STATE_CONFIGURING = CONFIGURING
Browser.STATE_EXECUTING = EXECUTING
Browser.STATE_READY_DISCONNECTED = READY_DISCONNECTED
Browser.STATE_EXECUTING_DISCONNECTED = EXECUTING_DISCONNECTED
Browser.STATE_DISCONNECTED = DISCONNECTED

But I think the code would be clearer if the initial state was CONNECTED rather than READY:

Browser.STATE_CONNECTED = CONNECTED
Browser.STATE_CONFIGURING = CONFIGURING
Browser.STATE_EXECUTING = EXECUTING
Browser.STATE_READY_DISCONNECTED = READY_DISCONNECTED
Browser.STATE_EXECUTING_DISCONNECTED = EXECUTING_DISCONNECTED
Browser.STATE_DISCONNECTED = DISCONNECTED

That clarifies the relationship with others states like EXECUTING_DISCONNECTED and avoids ambiguity about what the browser is "ready-for".

This version of the new state would be documented eg:

// The browser is connected but not yet been commanded to execute tests.
  const CONNECTED = 1

[solo-md]

I don't have any strong opinion here about naming.
State READY_DISCONNECTED is actually never used (and sounds confusing with the new states). I'll remove it.

[solo-md]

I also just came from vacation. Now the feedback-fix loop time should decrease.

[johnjbarton]

I think a slightly better description would be

// The browser has been told to execute tests; it is configuring before tests execution.

[johnjbarton]

I also just came from vacation. Now the feedback-fix loop time should decrease.

me too ;-)

[solo-md]

@johnjbarton I've refactored the state names as you've suggested.
State READY_DISCONNECTED is removed (because it was not used at all).
The browser.isReady() method was also refactored to browser.isConnected().

[johnjbarton]

Thanks!