Skip to content

Conversation

@boenrobot
Copy link
Contributor

Fixes #7

While the existing tests do pass, I'm not entirely sure how to make tests for this retry field yet... Just wanted to share the implementation. Some guidance to making tests for this will be appreciated.

@SimonFrings
Copy link
Contributor

@boenrobot thanks for bringing this up 👍

As a starting point, I think you will find similar tests inside the EvenSourceTest.php file. It's nearly the same approach as the tests for 'id' and 'event'. We're always happy about 100% code coverage ;)

Does this help to get started?

@boenrobot
Copy link
Contributor Author

Thanks.

I have added a test modeled after the last ID test.

@clue clue added the new feature New feature or request label Jan 31, 2022
@clue clue added this to the v1.0.0 milestone Jan 31, 2022
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@boenrobot Thanks for looking into this, love the direction!

Also great to see this already has 100% code coverage in its current form. I've briefly checked the specs and noticed we should probably also check for valid values. In particular, a non-integer value should be ignored as per https://html.spec.whatwg.org/multipage/server-sent-events.html#event-stream-interpretation

Can you add some additional tests to cover this in MessageEventTest?

@boenrobot
Copy link
Contributor Author

boenrobot commented Jan 31, 2022

Good catch. That scenario was broken.

I've added a test with an invalid value (that initially failed; the retry timer was set to 0), added a check that would accept only ASCII digits as valid, and would cast the value to either int or float in case the message's value to int or float (if the retry value is beyond PHP_INT_MAX; Not very likely, I know, but still, the spec doesn't impose limits and React supports float timers, i.e. more than PHP_INT_MAX).

Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@boenrobot Thank you for looking into this and your high-quality changeset, keep it up! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Respect retry field to change reconnection timer

3 participants