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

Added global beforeEach nemo injection #94

Merged
merged 6 commits into from
Jun 3, 2020

Conversation

Noyabronok
Copy link
Contributor

@Noyabronok Noyabronok commented May 2, 2020

Fixes/Features

  • Instantiates and injects nemo into context on the global beforeEach hook when driverPerTest is true
  • Updated dependency to Mocha 7.1.1 since that is the version that fixes start event firing too early, before it can be monitored. unable to listen to start event during programmatic run mochajs/mocha#2753
  • Added tests for global beforeEach hook. All tests pass
  • Readme updated
  • Version updated to 4.12.0

@grawk
Copy link
Member

grawk commented May 2, 2020

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.

@Noyabronok
Copy link
Contributor Author

@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.
Copy link
Member

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?

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 must have missed it. Which property is that?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grawk OK, I tried to replace instanceId with uid in

log('Instantiated new nemo instance', nemo.instanceId);
and got undefined. Where and when exactly is this uid defined? I think it might only be available for events, not nemo object itself.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

@grawk grawk May 9, 2020

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.

Copy link
Contributor Author

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!

@grawk grawk merged commit 9d7c0b6 into krakenjs:master Jun 3, 2020
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.

None yet

2 participants