Skip to content

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Nov 16, 2020

We're currently waiting 1s, which may not be enough time on slower CI machines. Increases timeouts to 60s. Ideally we'd just have no timeout at all here, but if it takes more than a minute for these conditions something else must be wrong.

@jmagman

This will be landed on red to help make the tree green - current redness is due to #70630

Fixes #70630

@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Nov 16, 2020
@google-cla google-cla bot added the cla: yes label Nov 16, 2020
@dnfield dnfield requested a review from jmagman November 16, 2020 18:17
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

Can we start with 5.0 and file an issue if it's taking longer than that?
The code LGTM to green out the build though.

@dnfield
Copy link
Contributor Author

dnfield commented Nov 16, 2020

I'd really rather just remove the timeout altogether.

This should never take more than 60s, but really we should just use the whole test timeout to find out exactly what's wrong. I just don't want to keep incremently moving this up all week as it continues to flake :)

@dnfield dnfield merged commit 90ae093 into flutter:master Nov 16, 2020
@dnfield dnfield deleted the deflake branch November 16, 2020 18:21
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

If this truly fixes your issue that would mean that the bots are super slow for some reason. Maybe we could make a timeout mechanism that is tied to to a metric that would scale with a slow bot like waitForRenderedFrames or waitForAllEventsToExecute. The latter wouldn't be hard to implement, just pop an event on the main queue. Run the main event queue for a couple of milliseconds then check if your event has been executed yet.

@jmagman
Copy link
Member

jmagman commented Nov 16, 2020

I'd really rather just remove the timeout altogether.

You mean make make the wait [NSDate distantFuture].timeIntervalSinceNow? That could hit a different timeout and cause flakes.

The latter wouldn't be hard to implement, just pop an event on the main queue. Run the main event queue for a couple of milliseconds then check if your event has been executed yet.

I think that's essentially what -waitForExistenceWithTimeout is doing--it's spinning the runloop until the element is rendered, and it stops spinning as soon as it does.

@dnfield
Copy link
Contributor Author

dnfield commented Nov 16, 2020

The bots are slow - sometimes.

The bot that failed this actually passed it again on a retry with the 1s timeout. I'm just going with a generous value that should avoid the need to revisit this - really we try to avoid timeouts in general except for the main test/bot timeout.

@gaaclarke
Copy link
Member

It's not a huge deal but I don't think my point was conveyed. Let me try saying it another way. When making a game you have 2 different ways to progress time for an event. You can base it off of the clock or you can base it off the update rate.

void Update() {
  _progress += 1;
}
void Update() {
  _progress += _rate * (Time.current - Time.lastFrameTime);
}

Tying your event to the real clock means that a drop in framerate will make sure your event still lasts the same amount of real time. Tying your event to the update event means that even when the framerate drops your event will progress with the same steps with time dilation.

We are effectively performing our wait like an event based on real time, that's how Apple has implemented it. In our case, with bots that are sometimes slow, time dilation is a good thing. The proposed wait mechanism would be more like basing our wait on the update calls and would experience time dilation with a slow execution.

Not a huge deal, I just want to make sure my point was communicated and understood.

@dnfield
Copy link
Contributor Author

dnfield commented Nov 16, 2020

How would we achieve what you're suggesting with an XCUITest?

@gaaclarke
Copy link
Member

My first 2 thoughts were tying the duration of the waiting to the framerate of the device (which should slow down on slow devices) or basing the waiting on flushing the whole event loop, which would also slow down on slow devices.

@dnfield dnfield mentioned this pull request Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

testFlutterViewWarm flake

3 participants