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

Add functional integration tests #54

Merged
merged 2 commits into from
Dec 30, 2016
Merged

Conversation

clue
Copy link
Member

@clue clue commented Dec 29, 2016

This simple PR aims to add a functional integration test (which also happens to be a prerequisite for implementing #24).

Unfortunately, this simple change introduces an indirect cyclic dependency between socket -> socket-client -> dns -> socket which causes the test setup to fail. I'm opening this PR in the hope to propose and discuss a possible solution.

@clue
Copy link
Member Author

clue commented Dec 29, 2016

Here's a dependency graph that shows this cyclic dependency (generated via https://github.com/clue/graph-composer):
socket

The react/dns package currently has a dependency on this root package (react/socket) and thus has no way of knowing which version we're currently at.

Note that this only applies when this is the root package, i.e. it does happen in a local test setup but will not happen when you use this project as a library and add any of these packages as a normal dependency.

One possible way to avoid this is to explicitly define the root package version as per https://getcomposer.org/doc/03-cli.md#composer-root-version:

$ COMPOSER_ROOT_VERSION=`git describe --abbrev=0` composer update

@clue clue changed the title [WIP] Add functional integration tests Add functional integration tests Dec 29, 2016
@clue
Copy link
Member Author

clue commented Dec 29, 2016

PR updated and ready for review :shipit:

@@ -144,6 +145,23 @@ $ composer require react/socket:^0.4.4

More details about version upgrades can be found in the [CHANGELOG](CHANGELOG.md).

## Tests

The run the test suite, you first need to clone this repo and then install all
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "To run..."

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.

Accept from that one typo LGTM :shipit:

We currently have an indirect cyclic dependency between
socket -> socket-client -> dns -> socket
which causes the test setup to fail otherwise.
@clue clue force-pushed the integration-tests branch from 99e37e9 to 8740211 Compare December 29, 2016 22:03
@clue
Copy link
Member Author

clue commented Dec 29, 2016

Thanks for spotting! Amended the last commit :shipit:

@clue clue merged commit 5d9f7fc into reactphp:master Dec 30, 2016
@clue clue deleted the integration-tests branch December 30, 2016 11:04
clue pushed a commit to clue-labs/socket that referenced this pull request Mar 29, 2017
First class support for PHP7 and HHVM
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.

3 participants