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

question, test: best way to retry async task #13346

Closed
refack opened this issue May 31, 2017 · 5 comments
Closed

question, test: best way to retry async task #13346

refack opened this issue May 31, 2017 · 5 comments
Labels
question Issues that look for answers. test Issues and PRs related to the tests.

Comments

@refack
Copy link
Contributor

refack commented May 31, 2017

  • Version: master
  • Platform: *
  • Subsystem: test

In some tests we want to delay or retry an async task whose exact timing for success is indeterminable.
A few option come to mind:

  1. setTimeout(task, n) where n is an arbitrary number that's empirically proven to be sufficient.
  2. const interval = setInterval(() => {const ret = task(); if (ret) clearInterval(interval);}), 1)
  3. do { const ret = await task()} while (ret != true)

Any other ideas or preferences?
Ref: #13252
Ref: #13312

/cc @nodejs/testing

@Trott
Copy link
Member

Trott commented May 31, 2017

I dislike 1 because it results in tests that are unreliable under load.

I like 2. People sometimes seem surprised when they see it, but it has yet to cause problems.

I don't have a problem with option 3 for the most part, but a potential issue with number 3 is that it makes tests unrunnable in older versions of Node.js. Often, when refactoring a test, I'll run it in an older version where it is expected to fail to make sure it still fails. This is not an issue for newer tests, of course.

@refack
Copy link
Contributor Author

refack commented May 31, 2017

Should we add a helper to common something like: common.tryUntil(block, pred)
where block can be a ((cb) => {})((ret) => if (pred(ret)) stop)
or an awaitable factory block().then((ret) => if (pred(ret)) stop)

Or we can document 2 as an idiom (and 3 is self explanatory)

@mscdex mscdex added question Issues that look for answers. test Issues and PRs related to the tests. labels May 31, 2017
@Trott
Copy link
Member

Trott commented May 31, 2017

I prefer documenting the idiom to a new helper function. My bar for adding yet more stuff to common is pretty high.

@Trott
Copy link
Member

Trott commented Aug 7, 2017

Should this remain open?

@refack
Copy link
Contributor Author

refack commented Aug 7, 2017

Closing, as this should be turned into PR.

@refack refack closed this as completed Aug 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Issues that look for answers. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

3 participants