Idempotent #58
Idempotent #58
Conversation
|
|
||
| $body = $channel->__publish_request_body( $msg ); | ||
| $body = json_decode($body); | ||
| $this->assertEquals( $body->id, "foobar" ); |
There was a problem hiding this comment.
This is an inadequate test, why are you not doing a history query as well to check that the outcome is as expected?
| $id = explode ( ":", $body->id); | ||
| $this->assertEquals( count($id), 2); | ||
| $this->assertGreaterThanOrEqual( base64_decode($id[0]), 9); | ||
| $this->assertEquals( $id[1], "0"); |
There was a problem hiding this comment.
Can you not do a history request to get this info?
| $body = json_decode($body); | ||
|
|
||
| $this->assertEquals( $body[0]->id, "foobar" ); | ||
| $this->assertFalse( property_exists($body[1], 'id') ); |
There was a problem hiding this comment.
This should fail, you can't mix messages with idempotency and not idempotency, so not sure why this works.
There was a problem hiding this comment.
Don't understand. RSL1k3 says If more than one Message is passed to publish() and one or more of those messages contains a non-empty id attribute, then all message ids (present or absent) are preserved on sending the batch of messages.
That's what we're testing. The first message with id, the second without. In line 105 we test the id is sent for th 1st message, in line 106 we test it's not sent for the 2nd message.
There was a problem hiding this comment.
Yes, @jdavid from a client SDK perspective, this is true. However, from a realtime perspective, we are meant to reject publishes for batches of messages where one or more messages contain IDs that don't follow the ID format described in RSL1k1. So this is a realtime bug, but I am concerned that once that bug is fixed, these tests will start failing.
There was a problem hiding this comment.
@paddybyers see above. Could we implement a quick fix to at least reject those messages for now?
There was a problem hiding this comment.
Just run the tests locally and they still pass, so I guess the quick fix has not been implemented?
There was a problem hiding this comment.
I've just written a test that sends an identical 2 messages and they get rejected. @jdavid are you testing this against the idempotent-dev environment?
There was a problem hiding this comment.
@paddybyers yes, in the idempotent branch, since commit 2e04a82
There was a problem hiding this comment.
@jdavid ok then, how easy is it for me to get set up to run the tests myself to see what's going on?
|
Has idempotent support been dropped from the |
|
RSL1k4 done, the PR is complete. But tests are failing. At least some of the failures are because, apparently, the dev environment doesn't support idempotency anymore. |
Seen in Travis: PHP Warning: Declaration of tests\HttpMockIdempotent::request($method, ...$args) should be compatible with Ably\Http::request($method, $url, $headers = Array, $params = Array) in /home/travis/build/ably/ably-php/tests/ChannelIdempotentTest.php on line 22
|
Tests are green now. @mattheworiordan I think I've addressed your first 2 comments, but I need clarification on the third one. Otherwise the PR is complete. |
|
ping |
|
@jdavid when you say the third one, I assume you are referring to #58 (comment). If so, I am afraid I am waiting on @paddybyers. I will ping him again too. |
|
Better yet. Can you not just provide an http raw log?
On Wed, 17 Oct 2018 at 12:53, Paddy Byers ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/ChannelIdempotentTest.php
<#58 (comment)>:
> + $msg = new Message();
+ $msg->name = 'name';
+ $msg->data = 'data';
+ $msg->id = 'foobar';
+ $messages[] = $msg;
+
+ $msg = new Message();
+ $msg->name = 'name';
+ $msg->data = 'data';
+ $messages[] = $msg;
+
+ $body = $channel->__publish_request_body( $messages );
+ $body = json_decode($body);
+
+ $this->assertEquals( $body[0]->id, "foobar" );
+ $this->assertFalse( property_exists($body[1], 'id') );
@jdavid <https://github.com/jdavid> ok then, how easy is it for me to get
set up to run the tests myself to see what's going on?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#58 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AACrDeB72K3Udnpna_hNGu7Fcz3iZ8cVks5ulxougaJpZM4Wg8G5>
.
--
Matthew O'Riordan <https://linkedin.com/in/lemon>
Ably CEO & tech co-founder <https://www.ably.io/about/team>
OpenAPI Board Member <https://www.openapis.org/membership/members>
github <https://github.com/mattheworiordan> / twitter
<https://twitter.com/mattheworiordan> / linkedin
<https://linkedin.com/in/lemon>
[image: Logo] <https://www.ably.io/>
Recent blog:
<https://blog.ably.io/boosting-developer-experience-on-our-website-with-a-major-facelift-a122e7bb4ce7>Boosting
Developer Experience
on our website with a major facelift
<https://blog.ably.io/boosting-developer-experience-on-our-website-with-a-major-facelift-a122e7bb4ce7>
|
|
Sorry the test was not actually publishing to the server, now it does and it is rejected. Now is it good? |
Looks good to me, thanks |
Not ready for merge as only 1 item is done (TO3n), but prefer to get early feedback.