async-interop / event-loop

An event loop interface for interoperability in PHP.
MIT License
169 stars 9 forks source link

Provide tests #60

Closed bwoebi closed 8 years ago

bwoebi commented 8 years ago

As we approach stabilization of the API... Do we want to provide tests here which every implementation must pass?

This would ensure that implementations are compatible between each other, at least on the level of the tests.

In case we can agree here, I'd take the Amp tests and update them to match the Loop API. (as differences are rather small). This would be then an abstract static class with an abstract function to get the respective Driver instance.

Note: Tests are not required for a first v1.0.0, they may still be included in a minor version later...

joshdifabio commented 8 years ago

I personally think that'd be great to have. Maybe it should be in a separate repo to the event-loop spec but still within this org?

bwoebi commented 8 years ago

@joshdifabio Is there any particular reason why tests cannot be in this repo? Their versioning is closely bound to the Loop(Driver) classes/interfaces?

In case we update the one we also have to update the other and vice versa.

joshdifabio commented 8 years ago
  1. I would have thought the tests would be likely to change more often than the interfaces, but I might be mistaken
  2. Only loop implementors would need the tests, whereas any async project would need the interfaces

I'm just asking the question, really, I'm not super convinced which approach is best.

Edit: To clarify what I mean in 1: would it be reasonable to expect that as time goes by the test suite would grow and improve?

bwoebi commented 8 years ago
  1. If the tests change, that's either a BC break, worth of a major version, or if we just add tests (without behavior change), it's just a minor version not breaking any semver compatible composer constraint, and thus not problematic.
  2. With that argumentation every repository ever shall separate its tests and logic. Having like one file more with tests which is not loaded under normal (except testing obviously) circumstances is doing absolutely no harm?
kelunik commented 8 years ago

With that argumentation every repository ever shall separate its tests and logic. Having like one file more with tests which is not loaded under normal (except testing obviously) circumstances is doing absolutely no harm?

They should be separate as any project separates them. They're usually in test and are not pulled when the repository is pulled as dependency (excluded via .gitattributes). Only when the repository is cloned form source.

Nobody needs these tests in vendor, they should be a require-dev for loop implementations, not a require.

bwoebi commented 8 years ago

@kelunik I meant, no projects separates its tests into a different repository, sorry if this wasn't clear.

kelunik commented 8 years ago

Sure, I got that. But this would not be tests for this repository, but other repositories. They shouldn't be in the standard definition.

joshdifabio commented 8 years ago

If the tests stay in this repo then it won't be possible to use the require-dev Composer config option to specify the dependencies of the async-interop/event-loop tests.

Currently, the composer.json file looks like this:

{
    "name": "async-interop/event-loop",
    "require-dev": {
        "phpunit/phpunit": "^4|^5"
    }
}

If some third party loop implementation, acme/event-loop, does the following, with the intention of running the async-interop/event-loop tests:

{
    "name": "acme/event-loop",
    "require": {
        "async-interop/event-loop": "^1.0"
    }
}

acme/event-loop won't get PHPUnit installed, it'll have to specify that in its own composer.json file (and any other dependencies of the async-interop/event-loop tests).

The following also wouldn't be a good solution, because it would cause PHPUnit and all of its dependencies to be included in production builds, and it would also fail on projects which have requirements which conflict with the dev requirements of async-interop/event-loop:

{
    "name": "async-interop/event-loop",
    "require": {
        "phpunit/phpunit": "^4|^5"
    }
}
AndrewCarterUK commented 8 years ago

I think these tests should be in a separate repository. Probably:

async-interop/event-loop-driver-tests

bwoebi commented 8 years ago

I've created the event-loop-tests repository. I'll push something in an hour or so.

bwoebi commented 8 years ago

Closing here, if anything needs to be discussed on this topic, please do this in the event-loop-tests repository.

I've also pushed the first tests. More tests are always welcome there.