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

Support for windows development #236

Open
Saturate opened this issue Feb 25, 2018 · 3 comments
Open

Support for windows development #236

Saturate opened this issue Feb 25, 2018 · 3 comments

Comments

@Saturate
Copy link
Contributor

This package works great on windows, however as it is right now it's not testable on windows. As seen in #234 this can course some issues when trying to contribute with a windows machine.

For windows support, the first step is to make all the tests support windows, and then enable a appveyor build, to keep it tested and supported.

This issues is tracking this progress and discussion. I intend to take a look at this at some point.

@pocketjoso
Copy link
Owner

pocketjoso commented Feb 25, 2018

Great initiative. 👍

For most tests it is hopefully just cleaning up path and process usage to make sure it works on windows as well, but the node module tests (for running multiple penthouse calls in parallel) that currently in a very hacky way rely on process path matching will need to completely be re-written. I only started thinking about it that a more sane way to test these patterns (was the same browser used) would be with test spies, i.e. just count how many times the critical methods/functions were called.

Note in general that I have been touching quite a lot of the core code recently just to play around with running more processes in parallel to increase performance. To update the tests it should not be needed to look at the core code, but headsup just in case.

@Saturate
Copy link
Contributor Author

Saturate commented Feb 25, 2018

So far I have had luck with __dirname, this seems to replace the intention of process.env.PWD.

+return 'file://' + path.join(__dirname, 'static-server', file)
-return 'file://' + path.join(process.env.PWD, 'test', 'static-server', file)

This alone made all tests pass on windows, so I guess supporting it is really simple. Should I just create a PR with this, and maybe a AppVeyor config?

Edit: All the tests in appveyor , so not done yet.

@pocketjoso
Copy link
Owner

Hmm I had issues with __dirname when I switched to the Jest taskrunner, that's the reason I switched. But I didn't look into it deeply so it should be possible to revert and fix the problem I had (relative paths making tests fail) in another way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants