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

Test Mac OS X on Travis #130

Merged
merged 2 commits into from
Oct 23, 2017
Merged

Test Mac OS X on Travis #130

merged 2 commits into from
Oct 23, 2017

Conversation

clue
Copy link
Member

@clue clue commented Oct 22, 2017

Squashed by @clue, originally from #124


Resolves / closes #121

Squashed by @clue, originally from reactphp#124
@clue clue changed the title [WIP] Test Mac OS X on Travis Test Mac OS X on Travis Oct 23, 2017
@clue
Copy link
Member Author

clue commented Oct 23, 2017

I've updated this to skip platform-specific (Linux) tests on other platforms (Mac OS X). This is now ready for review :shipit:

@andig
Copy link
Contributor

andig commented Oct 23, 2017

I've updated this to skip platform-specific (Linux) tests on other platforms (Mac OS X). This is now ready for review

Alternatively, you could mark the tests with @group and have the OSX or non-Linux build run with --excludeGroup. It's not quite clear why exactly these two tests should be run on Linux only (and not e.g. OSX)?

@clue
Copy link
Member Author

clue commented Oct 23, 2017

These two tests specifically test system behavior ("how does the server side react to when a client connection is started and aborted immediately without waiting for the connection to be fully established?") and do not really test things specific to this library at all. We know this is how Linux behaves but do not have any reliable source for other platforms, so I think it makes sense to just skip this for now other platforms until we have a better understanding of this.

Our library is known to work perfectly fine on Mac OS X and I think we should emphasize this (failing system specific tests give a wrong impression here). However, our resources to test this on other platforms are limited, I for one for not have a Mac at hand. Given how long this has gone unnoticed, I think it should be sufficient to rely on Travis to spot future regressions here.

@andig I think using a @group makes sense for tests that can/should explicitly be toggled. In this particular case I think it's a sane default to always skip this for Mac OS X 👍

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

:shipit:

@WyriHaximus WyriHaximus merged commit 25a1c27 into reactphp:master Oct 23, 2017
@WyriHaximus
Copy link
Member

@clue @jsor Do we want to test against OS X on all our other packages as well?

@clue clue deleted the macosx branch October 23, 2017 21:26
@clue
Copy link
Member Author

clue commented Oct 23, 2017

@WyriHaximus I don't really have a use for this unless anybody brings up any issues, but if you feel it's worth it, I'd say for got it 👍

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.

4 participants