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

Issue #1544 and #1446 Windows Compatibility #1545

Merged
merged 12 commits into from Jan 25, 2017
Merged

Conversation

ronan
Copy link
Contributor

@ronan ronan commented Jan 20, 2017

No description provided.

@ronan
Copy link
Contributor Author

ronan commented Jan 20, 2017

Also: #1446

@ronan ronan changed the title Issue #1544 Guard against missing posix_isatty function Issue #1544 and #1446 Windows Compatibility Jan 20, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.508% when pulling f8288f6 on 1544-windows-istty into 173f720 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.508% when pulling f8288f6 on 1544-windows-istty into 173f720 on master.

Copy link
Contributor

@TeslaDethray TeslaDethray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 @joshkoenig had the same idea for DefaultsConfig in #1546

@ronan
Copy link
Contributor Author

ronan commented Jan 20, 2017

I don't really want to merge this until I can validate that it fixes issues on Windows. All I can really say right now is it doesn't break things on a Mac

@greg-1-anderson
Copy link
Member

If no one else is working on this, I can try to set up Apveyor testing by adapting the config file Drush uses.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling f05435d on 1544-windows-istty into 4174a93 on master.

@greg-1-anderson
Copy link
Member

greg-1-anderson commented Jan 21, 2017

The appveyor config file I added here works -- I have Windows tests running on my fork of Terminus. I don't seem to have permissions to start building pantheon-systems/terminus on Appveyor, though. Not sure why that is.

In the first run that completed, there were ~13 failures. A lot of these were false positives, where the tests were expecting a path to come back as this/that, but it instead was this\that. A couple were due to calling escapeshellarg, which does not do the right thing on Windows. I pushed another commit here to fix that.

This fix improved things somewhat, but there are still 13 failures on the latest Appveyor run.

n.b. This is just the phpunit tests. I'm not sure how well the Behat tests will run.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling 6a17ac3 on 1544-windows-istty into 4174a93 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling ec3682f on 1544-windows-istty into 4174a93 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling e0a7fb8 on 1544-windows-istty into 4174a93 on master.

@greg-1-anderson
Copy link
Member

Got the phpunit tests working, save for two plugin tests which I just marked as "skipped".

Next we'll see how the Behat tests fare. Perhaps those can be put off until later, if they aren't easy to fix.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling a1f4d98 on 1544-windows-istty into 3f84d9a on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling ed4d734 on 1544-windows-istty into 3f84d9a on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling 087c8d6 on 1544-windows-istty into 3f84d9a on master.

@greg-1-anderson
Copy link
Member

Windows doesn't like setting environment variables in front of exec'ed commands, bash-style. The failing behat tests are here.

I think we'll have fairly reasonable coverage on Windows if we run only the phpunit tests on that platform. If we discover additional things not working on Terminus on Windows, then we can add more unit tests to cover those use cases.

I'll just remove Behat from Appveryor; then we should be all green here.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling 087c8d6 on 1544-windows-istty into 3f84d9a on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.517% when pulling eb1c50a on 1544-windows-istty into 3f84d9a on master.

@greg-1-anderson
Copy link
Member

Woot! Appveyor tests green!

@greg-1-anderson
Copy link
Member

Travis build wedged; I restarted it. Should turn green in a bit.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 89.917% when pulling ad0efd0 on 1544-windows-istty into d0606a1 on master.

@greg-1-anderson
Copy link
Member

Travis is back to green, but that's a particularly painful 0.03% drop in coverage. ;)

@TeslaDethray
Copy link
Contributor

🎻 🌧 😭 @greg-1-anderson

@greg-1-anderson
Copy link
Member

@TeslaDethray At least the badge rounds it up to 90. :)

@TeslaDethray TeslaDethray added this to the 1.0.1 milestone Jan 25, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 89.74% when pulling d80d6d5 on 1544-windows-istty into 993b131 on master.

@TeslaDethray TeslaDethray merged commit 9e39aa5 into master Jan 25, 2017
@TeslaDethray TeslaDethray deleted the 1544-windows-istty branch January 25, 2017 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants