-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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): allow updating total specs count #3264
fix(browser): allow updating total specs count #3264
Conversation
This change allows providing the total specs count after the tests have been started. The count will be updated in case it has been provided already. This can be uselful for some adapters where the total specs count can not be determined before starting the first test.
I guess the test is attempting to cover: However it tests something "not happening" and many things do not happen when code is changed. Maybe the event was originally |
lib/browser.js
Outdated
@@ -60,6 +60,8 @@ class Browser { | |||
|
|||
if (helper.isDefined(info.log)) { | |||
this.emitter.emit('browser_log', this, info.log, info.type) | |||
} else if (helper.isDefined(info.total)) { | |||
this.lastResult.total = info.total |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you guard the update to results with state EXECUTING
then the tests will pass. You can add a similar test with mock state EXECUTING
to verify the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to fix the test should ignore if browser not executing
I would suggest to wrap all those if-clauses with a check for state EXECUTING
.
Should this.refreshNoActivityTimeout()
be still called then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should log events in any state, they could be error messages for corner cases. So remove the line in the test that checks the .dump
path.
The this.refreshNoActivityTimeout()
should always be called, it signals that the browser is still on the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I've pushed my changes.
Thanks! |
Thanks for the quick handling 👍 |
This change allows providing the total specs count after the tests have
been started.
The count will be updated in case it has been provided already.
This can be uselful for some adapters where the total specs count can
not be determined before starting the first test.
Regarding tests
I wanted to update the tests and/or create a test case but the
onInfo
tests are not clear to me:So I would be happy to get feedback from you before writing a test case.
karma/test/unit/browser.spec.js
Lines 102 to 137 in c277a6b