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

Implement initial support for WSLv1 #172

Merged
merged 3 commits into from
Jun 1, 2020
Merged

Implement initial support for WSLv1 #172

merged 3 commits into from
Jun 1, 2020

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented May 23, 2020

This implementation is working for me locally without any edge cases, so wanted to open a PR for initial feedback on the structure.

I found during my implementing that it's fine for chromedriver to be called chromedriver.exe, which meant I didn't need to add any extra logic around extracting the file from the zip & then knowing to rename it.

I've not looked into the tests yet: unfortunately it's not really possible to write tests for WSL right now, as enabling WSL requires a restart which CIs & containers either don't support or crash on, but if you're ok with mocking/spying I can write some tests to make sure the right methods are called (which is how I've done it in other libraries).

closes #170

@kapoorlakshya
Copy link
Collaborator

but if you're ok with mocking/spying I can write some tests to make sure the right methods are called

@G-Rath That would be great! Thank you!

@G-Rath G-Rath marked this pull request as ready for review May 24, 2020 20:10
@lukepearcewp
Copy link

This is great! I was just having a load of trouble trying to get rspec playing nicely on a colleagues windows WSL setup and this just works :)

The only odd thing I did notice is that javascript console.log() messages are output to stdout which you don't normally see whilst running tests - not sure if this is avoidable?

@kapoorlakshya
Copy link
Collaborator

@G-Rath Thanks again for all the work! I'll try to review and hopefully merge it this week.

@kapoorlakshya
Copy link
Collaborator

@G-Rath The code looks good and the specs pass for me locally on WSL v1 (Windows 10 build 1903). Thank you again for your contribution! :)

P.S. I have created #173 to configure Appveyor (CI) with WSL to test the support long term. Please feel free to submit a PR for that if you're interested. Otherwise I'll get to it when I can.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jun 1, 2020

🥳 awesome! Happy to help - feel free to ping me on any WSL stuff that comes up (even if it's not a result of this PR).

Please feel free to submit a PR for that if you're interested. Otherwise I'll get to it when I can.

Oh wow that's cool! From everything I've read so far (admittedly not a lot outside of a few docker & github-actions feature-request-issues) it didn't seem like there was a CI that supported WSL testing as it requires a restart of the container to successfully complete setup which the CI host usually just says "must be finished" and prevents it from starting back up.

I've not used Appveyor before, but super keen to have a go at setting it up, since it could be used by a few of the other projects I've recently been implementing WSL support in.

Cheers!

@G-Rath G-Rath deleted the support-wsl-v1 branch June 1, 2020 06:23
@G-Rath
Copy link
Contributor Author

G-Rath commented Jun 1, 2020

Just remembered I meant to mention this but didn't: I'm pretty sure this logic should mostly "just work" for at least the Edge driver too, but haven't got it installed locally & we don't use it at work, so at some point when I've got the time I'll confirm if that's the case & open an issue or PR depending on how far I get.

(if the Appveyor setup works out, I'll probably do it after that as it'll make it easier anyway).

@kapoorlakshya
Copy link
Collaborator

@G-Rath Sounds good! Looking forward to more contributions from you.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jun 1, 2020

The only odd thing I did notice is that javascript console.log() messages are output to stdout which you don't normally see whilst running tests - not sure if this is avoidable?

@lukepearcewp so this is interesting: that doesn't happen when running in tmux, which is why I didn't notice it the first time.

So..., run it in tmux apparently is the nice answer?

I imagine that there is probably a --silent flag that it can be called with, which I'll look into when I've got a chance.


This also fixes an annoying terminal font change that happens with powershell, which I'm looking into ways around - I might make a PR to do something like trying to call the new powershell and if that doesn't exist use the old one, but it's good to know that tmux solves both those problems as there's no downside to running it in a tmux panel anyway 🤷

@lukepearcewp
Copy link

So..., run it in tmux apparently is the nice answer?

Ha, if it was my setup I wouldn't have noticed it either then ;)

Yeah it's not really a problem from my point of view, just something that stuck out as odd. I would look into it myself but I'm on a mac - trying to diagnose and fix stuff over a screen-sharing session on someone else's Windows setup isn't that fun :)

Anyways thanks again @G-Rath saved a load of headaches with this pull request.

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.

Support downloading exe when running WSL
3 participants