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

Clean up test suite and add a functional test case #24

Merged
merged 4 commits into from
Sep 6, 2015

Conversation

clue
Copy link
Member

@clue clue commented Apr 23, 2015

  • Clean up test suite
    • Move test helpers to base TestCase
    • Use Composer's autoload-dev
  • Add a functional test case

This rather small PR serves as the basis for my other upcoming PRs.


require __DIR__ . '/../vendor/autoload.php';

class TestCase extends \PHPUnit_Framework_TestCase
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason to place TestCase in the bootstrap rather then it's own file?

@WyriHaximus
Copy link
Member

Aside from the TestCase note: LGTM 👍

@clue
Copy link
Member Author

clue commented Jun 6, 2015

Thanks for your feedback @WyriHaximus, makes sense and has just been resolved! 👍

$deferred->reject($value);
return $deferred->promise();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

These 2 methods can be shortened to:

    protected function createPromiseResolved($value = null)
    {
        return Promise\resolve($value);
    }

    protected function createPromiseRejected($value = null)
    {
        return Promise\reject($value);
    }

which makes them kind of obsolete ;)

Copy link
Member

Choose a reason for hiding this comment

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

@jsor: 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, they are of little use, in particular given they're not even used as part of this PR :)

My initial motivation was to support react/promise v1 and v2 alike, but after talking to @jsor, we concluded that it's much cleaner to provide an intermediary v1.1 release to ease transition. Long story short: Let's keep this apart from this PR for now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

after talking to @jsor, we concluded that it's much cleaner to provide an intermediary v1.1 release to ease transition

Just for the reference, this is now being tracked in reactphp/promise#31

@clue
Copy link
Member Author

clue commented Jun 12, 2015

Thanks for the feedback @jsor and @WyriHaximus, everything's just been resolved 👍

@clue
Copy link
Member Author

clue commented Jun 30, 2015

Ping @WyriHaximus, @jsor

@jsor
Copy link
Member

jsor commented Jun 30, 2015

About the removal of tests/bootstrap.php: @cboden once opened reactphp/promise#17 to allow running the tests when installed as a dependency. Not sure this is still relevant.

@clue clue mentioned this pull request Aug 10, 2015
@clue
Copy link
Member Author

clue commented Aug 17, 2015

Not sure this is still relevant.

Personally, I would consider this a non-issue. After all, why would you need to run the tests of your dependencies from within your main project?

Admittedly, I don't really mind either way, let's just move this forward? :-)

Ping @cboden, @WyriHaximus

@WyriHaximus
Copy link
Member

Agreed let's more forward 👍

@jsor
Copy link
Member

jsor commented Aug 17, 2015

IIRC, this was primarily for running the component tests from the main react/react repository: https://github.com/reactphp/react/blob/master/phpunit.xml.dist#L16

@clue
Copy link
Member Author

clue commented Sep 5, 2015

Agreed let's more forward 👍

I think we all agree we can move forward either way.

Ping @cboden, what's your take on this?

@cboden
Copy link
Member

cboden commented Sep 6, 2015

I do like the ability to run all tests from the main repo. That can be applied in another PR though.

cboden added a commit that referenced this pull request Sep 6, 2015
Clean up test suite and add a functional test case
@cboden cboden merged commit 3fa027f into reactphp:master Sep 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants