Skip to content

Conversation

nilsreichardt
Copy link

This PR adds a --suite-load-timeout argument to remove the hard-coded timeout (see #2463). [More description is added later]


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@nilsreichardt
Copy link
Author

@natebosch I need a bit of feedback here. Would be a --suite-load-timeout option a good solution for this? Should we use a different name?

If you are not the right person for this, could you tag someone else?

After this, I will add tests for this PR 👍

/// The timeout for loading a test suite.
final Timeout? _suiteLoadTimeout;
Timeout get suiteLoadTimeout =>
_suiteLoadTimeout ?? const Timeout(Duration(minutes: 12));
Copy link
Author

Choose a reason for hiding this comment

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

I implemented --suite-load-timeout the same way as --ignore-timeouts was implemented. However, it's not optimal that we have two locations for the default value. Here and in pkgs/test_core/lib/src/runner/configuration/args.dart. Should we improve this? Or not? Or just writing a comment to update the default value also in the other file?

@nilsreichardt nilsreichardt marked this pull request as ready for review September 17, 2025 20:15
@nilsreichardt nilsreichardt requested a review from a team as a code owner September 17, 2025 20:15
@nilsreichardt
Copy link
Author

I'm marking this as ready for review in the hope of getting some feedback.

@natebosch
Copy link
Member

I think it would be fine to add a --suite-load-timeout option. I'm not sure if it needs to be in SuiteConfiguration or if it can live in Configuration alone. I'll also take a look at adding support in the yaml file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants