refactor(launcher): Refactoring EdgeBrowser to work properly with powershell scripts. #6

Itee · 2018-09-18T21:34:32Z

Next to #5 but with conformal commit comment.

Itee · 2018-09-18T21:37:14Z

The travis integration is still broken due to missing powershell.exe in targeted environment.

I have no idea on how i can fix that... Please help !

zzo · 2018-09-18T22:00:12Z

thanks but the commit message still doesn't match the conventions :)

zzo · 2018-09-18T22:04:06Z

there may be a way to tell travis to install powershell - I found this maybe that helps?

Itee · 2018-09-18T22:22:28Z

@zzo

thanks but the commit message still doesn't match the conventions :)

Please tell me, what's wrong ? 😖

The build pass on appveyor: karma-edge-launcher

Thanks for the link, i will take a look deeper !

zzo · 2018-09-18T22:36:08Z

look here
At the 'example commit message' section.
Should look something like:
feat(all of this cool stuff does cool things)
or
fix(fixing all the things)
....
thanks!

nickserv · 2018-09-18T22:56:07Z

You can rebase the PR instead of making a new one.git rebase --interactive master is probably the easiest way. Then edit the commit messages, save the file, and git push --force-with-lease.

Itee · 2018-09-19T08:29:56Z

@zzo from the doc you linked

The commit message should be in this format:

<type>(<scope>): <subject>

<body>

<footer>

Where <type> is one of:

feat (new feature for the user, not a new feature for build script)
fix (bug fix for the user, not a fix to a build script)
docs (changes to the documentation)
style (formatting, missing semi colons, etc; no production code change)
refactor (refactoring production code, eg. renaming a variable)
test (adding missing tests, refactoring tests; no production code change)
chore (updating grunt tasks etc; no production code change)

the (<scope>):

init
runner
watcher
config
web-server
proxy
etc.

The <scope> can be empty (e.g. if the change is a global or difficult to assign to a single component), in which case the parentheses are omitted. In smaller projects such as Karma plugins, the <scope> is empty.

The given example is:

fix(middleware): ensure Range headers adhere more closely to RFC 2616

Add one new dependency, use `range-parser` (Express dependency) to compute
range. It is more well-tested in the wild.

Fixes #2310

and mine is like:

test(launcher): Refactoring test to work properly with powershell scripts.

Move test file in launchers folder to allow proper include of hard coded dependencies. And add new test to check if the decorator pattern is correctly apply to the launcher.
Add test folder to npmignore file.

So maybe i'm a bit tired, but they look really close. To me the doc say that the scope corresponding to the component where changes appear or could be empty on little project, so not like:

feat(all of this cool stuff does cool things)

Or misunderstanding i what say the doc ? I won't update commit message to change them again and again. But yes i will remove parentheses from the last commit message. And make shorter subject on first commit.

@nickmccurdy Thanks for the git command i'm aware of rebase -i but i never need to use it before so i will try and pray that work like you want.

Itee · 2018-09-19T08:40:54Z

I think the build under travis will never pass... Even if powershell is install the Edge Browser will still be missing.

Is there a way to use continuous-integration under AppVeyor for this Windows only package ?

Use powershell scripts to start and stop Edge browser instead of using edge-launcher package. Refactor EdgeBrowser implementation to work using powershell scripts. It allow to close gracefully the Microsoft Edge browser. Fixes #4

…ipts. Move test file in launchers folder to allow proper include of hard coded dependencies. And add new test to check if the decorator pattern is correctly apply to the launcher. Add test folder to npmignore file.

Update the travis file to integrate build under windows environment and allow latest node js version

nickserv · 2018-09-19T14:28:20Z

Unfortunately I think AppVeyor is also missing Edge because it's running Windows Server. I worked around this by mocking out edge in the tests.

zzo · 2018-09-19T17:07:17Z

yes the description of the PR is fine - the TITLE of the PR 'This PR fix #4' needs to be 'test(launcher): Refactoring test to work properly with powershell scripts.'
:) thanks!

Itee · 2018-09-19T19:52:31Z

@zzo With pleasure ! 😁

zzo · 2018-09-19T22:02:55Z

so I just generated a 0.4.3 release but I cannot push to npm - @nickmccurdy or @watilde have permissions to do that.

Itee · 2018-09-19T22:19:56Z

Thanks a lot !

Just two little things.

First, the build still don't pass travis integration due to powershell dependency. Should i open an issus for that ?

Second, there is a little "pitfall" in start_edge script due to non invasive script implementation.

Here the piece of code in cause:

# Check if edge is already running and cancel run if one is found.
$MicrosoftEdgeProcess = Get-Process "MicrosoftEdge" -ErrorAction SilentlyContinue
$MicrosoftEdgeCPProcess = Get-Process "MicrosoftEdgeCP" -ErrorAction SilentlyContinue
if( $MicrosoftEdgeProcess -or $MicrosoftEdgeCPProcess ) {
    Write-Output "An instance of Windows Edge browser is already running. Please close it and try again."
    exit 1
}

Edge have only one single main process, and it is impossible ( for the moment ) to know if the running instance was open from the user to go on the internet or by default by Windows. I saw during my tests that Windows can pop a Microsoft Edge process without any opened frame of it.

In such case, maybe a force option could be helpful to close the detected running instance instead of just exit on error and let the user kill the process by hand ?

Itee · 2018-09-19T22:23:02Z

Or should we consider that when a user start karma-edge-launcher he is exposed to an untimely Edge browser close by default ?

nickserv · 2018-09-20T01:09:44Z

I'd like to get the Travis and AppVeyor build passing before a stable release if you don't mind. I can publish a prerelease or add you to npm (if you give your usernames) if it helps.

Can we at least detect if Edge is open before the tests start and then only kill it wasn't already open? This way we can keep automatically closing tabs for most users as with #4 without being too invasive for people that actually use Edge manually.

Itee · 2018-09-27T11:32:15Z

Unfortunately after some test i am unable to check if edge was open by user or by window itself... The only difference is that auto-openned is running in background.

Else...

About the travis build the only way that i found to be able to run test with integrated MicrosoftEdge browser is using BrowserStack following this step from travis setup and using config like this one taken from here

Have you any BrowserStack account ???

jslegers · 2019-04-24T17:51:10Z

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

Itee added 3 commits September 19, 2018 10:51

chore: Target travis to Windows Os

0accbf8

Update the travis file to integrate build under windows environment and allow latest node js version

Itee changed the title ~~This PR fix #4~~ test(launcher): Refactoring test to work properly with powershell scripts. Sep 19, 2018

Itee changed the title ~~test(launcher): Refactoring test to work properly with powershell scripts.~~ refactor(launcher): Refactoring EdgeBrowser to work properly with powershell scripts. Sep 19, 2018

zzo merged commit a24106d into karma-runner:master Sep 19, 2018

watilde mentioned this pull request Jan 8, 2019

Publish 0.4.3 on npm #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(launcher): Refactoring EdgeBrowser to work properly with powershell scripts. #6

refactor(launcher): Refactoring EdgeBrowser to work properly with powershell scripts. #6

Itee commented Sep 18, 2018

Itee commented Sep 18, 2018

zzo commented Sep 18, 2018

zzo commented Sep 18, 2018

Itee commented Sep 18, 2018

zzo commented Sep 18, 2018

nickserv commented Sep 18, 2018 •

edited

Loading

Itee commented Sep 19, 2018

Itee commented Sep 19, 2018

nickserv commented Sep 19, 2018

zzo commented Sep 19, 2018

Itee commented Sep 19, 2018

zzo commented Sep 19, 2018

Itee commented Sep 19, 2018

Itee commented Sep 19, 2018

nickserv commented Sep 20, 2018

Itee commented Sep 27, 2018

jslegers commented Apr 24, 2019

refactor(launcher): Refactoring EdgeBrowser to work properly with powershell scripts. #6

refactor(launcher): Refactoring EdgeBrowser to work properly with powershell scripts. #6

Conversation

Itee commented Sep 18, 2018

Itee commented Sep 18, 2018

zzo commented Sep 18, 2018

zzo commented Sep 18, 2018

Itee commented Sep 18, 2018

zzo commented Sep 18, 2018

nickserv commented Sep 18, 2018 • edited Loading

Itee commented Sep 19, 2018

Itee commented Sep 19, 2018

nickserv commented Sep 19, 2018

zzo commented Sep 19, 2018

Itee commented Sep 19, 2018

zzo commented Sep 19, 2018

Itee commented Sep 19, 2018

Itee commented Sep 19, 2018

nickserv commented Sep 20, 2018

Itee commented Sep 27, 2018

jslegers commented Apr 24, 2019

nickserv commented Sep 18, 2018 •

edited

Loading