-
Notifications
You must be signed in to change notification settings - Fork 110
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
Conversation
@G-Rath That would be great! Thank you! |
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? |
@G-Rath Thanks again for all the work! I'll try to review and hopefully merge it this week. |
@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. |
🥳 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).
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! |
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). |
@G-Rath Sounds good! Looking forward to more contributions from you. |
@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 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 🤷 |
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. |
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 calledchromedriver.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