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 #4 #5

Closed
wants to merge 27 commits into from
Closed

Fix #4 #5

wants to merge 27 commits into from

Conversation

Itee
Copy link
Contributor

@Itee Itee commented Sep 18, 2018

Hi there,

This PR will fix #4 but make some changes (non breaking i think) about the logic.

It use two powershell scripts instead of the edge-launcher package wrapped into start and exit handlers to start and stop gracefully edge process.

I improved the units tests too and update them accordingly to the new features.

Hope this help.
Expecting your review and have a nice day !

…and output error in case of reach this limit
…cessExit existance ( we are overriding the class so it will be defined )
…opied dependency that are required in hard coded way. Make big improvement of the implemented launcher interface using decorators
@zzo
Copy link
Contributor

zzo commented Sep 18, 2018

please fix the PR to match our conventions thanks!

@Itee
Copy link
Contributor Author

Itee commented Sep 18, 2018

Mmmh sorry for that, i wasn't aware of it. I will update the PR.

I got some trouble about travis builds... due to the environement of build. How could i tell to travis that powershell is a pre-requis ?

@Itee
Copy link
Contributor Author

Itee commented Sep 18, 2018

I will create a new PR with updated commit messages. So i close this one.

@Itee Itee closed this Sep 18, 2018
@nickserv
Copy link
Collaborator

I haven't tried Travis on Windows, it might be easier to get it working on AppVeyor: https://ci.appveyor.com/project/nickmccurdy/karma-edge-launcher-ui7ax

I'll be sure to check out your new PR as this project has been kind of stagnant with the lack of edge-launcher updates. Thanks for your contribution!

@Itee
Copy link
Contributor Author

Itee commented Sep 18, 2018

@nickmccurdy You're welcome ! I just hope this will work, and go in the right way. I will take a look to appveyor.

@jslegers
Copy link

I didn't get either 0.4.2 or 0.4.3 to work as expected.

Version 0.4.2 closed Edge correctly, but open tabs were re-opened during the next run. This caused Edge to have X open tabs after X runs.

Version 0.4.3 failed to close Edge. And next run, Edge refused to start because it already was open.

I combined what worked in 0.4.2 with what worked in 0.4.3 and added some paths of this script, and now everything works as expected in my test environment.

With my changes, Edge now correctly starts with just one tab open every test run, and gets closed correctly afterwards.

For my changes, see #9

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.

Edge does not close its tabs
4 participants