Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@cbracken
Copy link
Member

When run repeatedly, some of the iOS unit tests can relatively easily
blow their test timeouts. Since we're testing behaviour and not
performance in these tests there's no need to enforce a particular
timeout.

The bots will fail if the tests timeout, regardless.

Issue: flutter/flutter#157231

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

When run repeatedly, some of the iOS unit tests can relatively easily
blow their test timeouts. Since we're testing behaviour and not
performance in these tests there's no need to enforce a particular
timeout.

The bots will fail if the tests timeout, regardless.

Issue: flutter/flutter#157231
@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 21, 2024
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.

I'm for removing the timeouts to reduce flakes, but I'd prefer if we keep the waitForExpectationsWithTimeout behavior that doesn't require each expectation to be explicitly awaited. It's too easy to miss new ones.
Can we make the timeout ridiculously large instead?
Same comment for #55961

@jmagman jmagman removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 21, 2024
@cbracken
Copy link
Member Author

Yeah we could define a shared constant somewhere.

@hellohuanlin
Copy link
Contributor

I'd prefer if we keep the waitForExpectationsWithTimeout behavior that doesn't require each expectation to be explicitly awaited. It's too easy to miss new ones.

I'm surprised that Apple doesn't provide a version of API that doesn't take the timeout. Ya I agree that its very easy to forget some expectations this way.

@cbracken
Copy link
Member Author

cbracken commented Oct 22, 2024

I was hoping that Apple would have provided a way to look up all created expectations but it seems like they also don't provide that -- if they did, we could just gather up the expectations from wherever apple is gathering them up, then invoke the method I used.

Based on the documentation, the waitForExpectationsWithTimeout:handler: method looks like XCTestCase is a little shady -- note that it'll only capture expectations created via XCTestCase wrapper methods, and not those created directly as an XCTestExpectation, for example.

I actually personally prefer being explicit about what expectations we're creating and what expectations we're waiting for -- as a bonus, you can interleave creating and waiting for expectations -- but I don't feel super strongly about it, and if the consensus is we prefer to wait for everything, I'm more than happy to switch it back to that with a long timeout declared in a shared header.

@chinmaygarde
Copy link
Member

Can we land this now? Seems good to go.

@chinmaygarde
Copy link
Member

Adding a WIP tag so triagers don't trip on this. Please remove once progress is made.

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Nov 4, 2024
@cbracken
Copy link
Member Author

cbracken commented Nov 4, 2024

Thanks -- I'll get time to take a second pass at this this week.

@jmagman jmagman marked this pull request as draft November 6, 2024 22:10
@cbracken cbracken closed this Dec 20, 2024
@cbracken
Copy link
Member Author

Since the engine repo is no longer a thing, I'm closing this. Plus I'm going to take a different approach with this.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants