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

Make timing explicit in test #15

Conversation

eps1lon
Copy link

@eps1lon eps1lon commented Mar 5, 2021

@bvaughn I found the test slightly confusing because the timing didn't appear in the test i.e. I could change the delay of flushPendingErrorsAndWarningsAfterDelay to 10s or 5s and it still passed.

I copied the approach of asyncAct to not flush recursively so that we can visualize the timing in the test.

In the end we traded one implementation detail (advance timers by 1 to flush commit) for another (advance timers by 1000 to flush pending warnings). I'd argue we're more interested in the 1000ms delay here and it isn't just an implementation detail.

Feel free to close this if you don't think this improves readability of the test.

Ideally we'd have a dedicated method to flush bridge operations so that we don't run unrelated timers. Running timers implicitly is, to me, more confusing then having to advance it explicitly.

@bvaughn
Copy link
Owner

bvaughn commented Mar 5, 2021

Ideally we'd have a dedicated method to flush bridge operations so that we don't run unrelated timers. Running timers implicitly is, to me, more confusing then having to advance it explicitly.

Yeah. This sounds like it would be a nice improvement!

@bvaughn bvaughn merged commit 8716606 into bvaughn:devtools-inline-warnings-flush-passive-after-delay Mar 5, 2021
@bvaughn
Copy link
Owner

bvaughn commented Mar 5, 2021

Sure. Lemme pull this into my branch. I was planning on adding another test ths morning anyway :D

@bvaughn bvaughn deleted the devtools-inline-warnings-flush-passive-after-delay-test-explicit branch March 5, 2021 14:12
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.

2 participants