-
Notifications
You must be signed in to change notification settings - Fork 23
Conversation
Thanks so much for this PR, @Noyabronok. I'm going to submit a separate PR just to update .travis.yml and chromedriver in order to get some of the other stacked up PRs merged. It may require a rebase of this PR. Regarding a major version bump: I don't think it's necessary to do this, as the breaking change is only related to dropping support for a non-LTS version of Node.js. We should be able to do this change under a minor version. |
@grawk All done. Please review. Thanks! |
README.md
Outdated
context. | ||
|
||
### InstanceId | ||
|
||
Each nemo instance will be assigned an `instanceId` property, that you can use to uniquely identify a nemo instance. |
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.
We already have a uid
tag to identify Nemo instances. Can you explain why a new unique ID is needed here?
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 must have missed it. Which property is that?
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.
never mind, you said uid
I'll remove the new one.
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.
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.
@grawk I found the value in instanceConfig.profile.tags.uid
in runner/mocha.js
but it's not unique per actual nemo-core instance injected into mocha hook this
context. Seems to be more of a "Nemo Runner profile / parallel" scope lifetime, which can spawn multiple nemo-core instances.
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.
Thanks @Noyabronok .. I had to go familiarize myself with that code again. Can you please let me know what is your use case for this additional property on the nemo object?
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.
@grawk Well, I initially added it to easily understand and confirm the lifetime of the this.nemo
object. So, right now it's just referenced in logging. Maybe there are some other use cases, not sure. I thought it was nice to keep since I already added it.
While we're at it, I'd like to confirm my understanding of the tags.uid
property. Readme.md refers to it as an instance for a process. Seems like the purpose is to identify different process instances running in parallel. To me, that's not a Nemo instance, but more of a process instance, since each process can spawn many nemo-core instances, as I mentioned above.
But, I guess it's a matter of language. I see that Readme.md refers to each nemo-core instance as webdriver, as mentioned in the lifecycle section explaining driverPerTest
. So maybe, to keep things consistent with existing language, I should rename this.nemo.instanceId
to this.nemo.webdriverId
or this.nemo.driverId
?
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.
You're right. The term "nemo instance" is unfortunately a bit overridden by those two meanings. You could consider the current tags.uid
as representing a Mocha Runner
instance. And within that instance there could be multiple nemo
(WebDriver) instances.
My trepidation to adding nemo.instanceId
is it being a new property on the nemo
object, which is currently fairly sparse. Do you think it would be ok to take out the new property from this PR and discuss it in an issue or future PR?
That would clear the way to merge/publish the next version which takes advantage of the newly fixed Mocha start
event.
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.
@grawk I removed instanceId
from this PR. Please merge. Thanks!
Fixes/Features
nemo
into context on the globalbeforeEach
hook whendriverPerTest
istrue
start
event firing too early, before it can be monitored. unable to listen to start event during programmatic run mochajs/mocha#2753