Skip to content

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Dec 31, 2018

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
Documentation
License MIT

What's in this PR?

I found a bug. The bug is present all these are true:

  • We use a synchronous client
  • We use a synchronous request
  • We have a plugin that use return $first($request)->wait()

HOWEVER, using return $first($request)->wait() does not make any sense...
I got the idea from #103 but after writing this PR and #103 (comment) I discovered that there is no bug and one should never do return $first($request)->wait().

Im not sure we want to merge this test or not. I kind of like the code so I did not want to throw it away =)

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

this looks like a useful functional test to make sure we never mess up with either sync or async client.

@sagikazarmark is it reasonable to write such a functional test in phpspec, or does it make more sense to use phpunit for this? the point to me is that we don't mock but use actual clients and plugins.

@dbu
Copy link
Contributor

dbu commented Jan 2, 2019

btw, its not highlighting a bug anymore, lets change the description

@dbu dbu changed the title Added test to highlight a bug Functional test to ensure no issue with sync nor async requests Jan 2, 2019
@dbu dbu changed the title Functional test to ensure no issue with sync nor async requests Functional test to ensure no issue with plugins in sync nor async requests Jan 2, 2019
@sagikazarmark
Copy link
Member

To be honest, I think we should stop using phpspec. PHPSpec is great, but it requires too much hacks and results in absolutely not expressive tests in our case.

@dbu dbu merged commit 3feffd5 into 2.x Jan 3, 2019
@dbu dbu deleted the patch-unittest branch January 3, 2019 07:26
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.

4 participants