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

gloo-timers: don't consume callback in Interval closure #57

Merged
merged 2 commits into from
Mar 29, 2019

Conversation

samcday
Copy link
Contributor

@samcday samcday commented Mar 28, 2019

Otherwise Interval only works on first callback and fails after that

…nterval only works on first callback and fails after that
@samcday samcday mentioned this pull request Mar 28, 2019
@OddCoincidence
Copy link
Contributor

Could we also add a test that triggers this bug?

@samcday
Copy link
Contributor Author

samcday commented Mar 28, 2019

@OddCoincidence yep working on tests now. As it happens integration tests are not running in CI, which is why it's reporting that these changes are okay here, when in fact fixing the bug actually causes the interval_cancel test to fail.

@samcday
Copy link
Contributor Author

samcday commented Mar 28, 2019

@OddCoincidence alright I wrangled the tests. Hope you don't mind but I did a little bit of re-jiggery once I understood how they worked to simplify them a little.

The interval test now ensures that 5 ticks of the interval pass. interval_stream was already doing that so now they're consistent. I removed the Cell stuff in favour of just collect()ing the Stream and counting how many results we got.

The interval_cancel test started failing once I fixed the original bug. I reworked it a little to avoid the dance it was doing in favour of just combining a Timeout stream with the Interval stream we're examining.

@samcday samcday force-pushed the fix-timers-interval branch from 0fbfa4d to f7858fe Compare March 28, 2019 17:57
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great with one nitpick to fix before we merge this; thanks @samcday!

crates/timers/tests/web.rs Outdated Show resolved Hide resolved
@samcday samcday force-pushed the fix-timers-interval branch from f7858fe to 1bd3f75 Compare March 29, 2019 07:16
@samcday samcday force-pushed the fix-timers-interval branch from 1bd3f75 to 7749e20 Compare March 29, 2019 07:17
@samcday
Copy link
Contributor Author

samcday commented Mar 29, 2019

Sweet, feedback addressed. Looks like CI is running headless tests now too, nice!

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

@fitzgen fitzgen merged commit e9a3fa3 into rustwasm:master Mar 29, 2019
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