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

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

Merged
merged 3 commits into from
Jul 23, 2018
Merged

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

merged 3 commits into from
Jul 23, 2018

Conversation

solo-md
Copy link
Contributor

@solo-md solo-md commented Jul 5, 2018

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

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

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
Corporate signers

@solo-md
Copy link
Contributor Author

solo-md commented Jul 6, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Sorry the vacation delay....

@@ -101,6 +101,8 @@ class Browser {
this.lastResult = new Result()
this.lastResult.total = info.total

this.state = EXECUTING

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

The CONFIGURING state means that the browser is already not READY for tests, as someone has requested tests execution (and provided a config file). But the provided config file is not yet processed, configuration is not applied and the tests execution is not yet started, so the browser is not yet at EXECUTING state.
@solo-md
Copy link
Contributor Author

solo-md commented Jul 23, 2018

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

lib/browser.js Outdated
@@ -7,17 +7,20 @@ const logger = require('./logger')
// The browser is ready to execute tests.
const READY = 1

// The browser is configuring before tests execution.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a slightly better description would be

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

@johnjbarton
Copy link
Contributor

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

me too ;-)

@solo-md
Copy link
Contributor Author

solo-md commented Jul 23, 2018

@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().

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

3 participants