search

karma-runner / karma-edge-launcher

A Karma plugin. Launcher for Microsoft Edge.
MIT License
21 stars 7 forks source link

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

Closed Itee closed 6 years ago

Itee commented 6 years ago

Next to #5 but with conformal commit comment.

Itee commented 6 years ago

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 commented 6 years ago

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

zzo commented 6 years ago

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

Itee commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@zzo from the doc you linked

The commit message should be in this format:

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

<body>

<footer>

Where <type> is one of:

the (<scope>):

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 commented 6 years ago

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 ?

nickserv commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@zzo With pleasure ! 😁

zzo commented 6 years ago

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

Itee commented 6 years ago

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 commented 6 years ago

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

nickserv commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 https://github.com/karma-runner/karma-edge-launcher/pull/9