Skip to content

Add patrol tests #420

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

Merged
merged 4 commits into from
Feb 14, 2025
Merged

Add patrol tests #420

merged 4 commits into from
Feb 14, 2025

Conversation

piotruela
Copy link
Contributor

@piotruela piotruela commented Nov 3, 2024

This PR add testing 2 packages that are part of the Patrol testing tool:

They relies on multiple dependencies pinned by the Flutter SDK, most heavily on the flutter_test API.

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 the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@piotruela piotruela mentioned this pull request Nov 3, 2024
8 tasks
@kevmoo
Copy link

kevmoo commented Nov 3, 2024

Uh... @devoncarew ? @natebosch ?

@devoncarew
Copy link
Member

I'm curious - how much of package:test_api is used? I believe that's currently very lightly used, and mostly by flutter test. Adding a test here for a package that uses it might effect our ability to evolve that api.

@natebosch
Copy link

It seems like this is primarily a version-change detection test for the test_api package? I don't think we have made, nor should yet make, any promises about not bumping to a new major version of test_api.

I do think that as we iterate further on the extensibility of the test runner, package:test_api will likely become a dependency in more places.

@piotruela
Copy link
Contributor Author

@devoncarew @natebosch I get your point. I removed from the tests the package that directly depends on test_api and updated PR description.

@Piinks
Copy link
Contributor

Piinks commented Jan 8, 2025

@piotruela it looks like this fell through our review cracks, sorry about that. Would you like to continue with his change and rebase this PR?

@piotruela
Copy link
Contributor Author

@Piinks Sorry for the late reply. Yes, I'll rebase it asap.

@piotruela piotruela force-pushed the add-patrol-tests branch 2 times, most recently from f097834 to cbcaf54 Compare February 5, 2025 20:37
Piinks
Piinks previously approved these changes Feb 11, 2025
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

This LGTM thank you! A change in flutter/flutter will be needed to update the pinned version of flutter/tests after this lands (so flutter/flutter will actually run these tests!)

chunhtai
chunhtai previously approved these changes Feb 11, 2025
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

auto-submit bot commented Feb 11, 2025

autosubmit label was removed for flutter/tests/420, because - The status or check suite windows_tests (shard 5 of 5, 4) has failed. Please fix the issues identified (or deflake) before re-applying this label.

@Piinks
Copy link
Contributor

Piinks commented Feb 11, 2025

Hmm, it looks like this is failing some checks. @piotruela can you try rebasing with the tip of tree here to see if it resolves the issues?

@piotruela piotruela dismissed stale reviews from chunhtai and Piinks via 4e2eb3d February 12, 2025 19:39
@piotruela
Copy link
Contributor Author

@Piinks I've rebased the PR and also updated the script file to work on multiple platforms.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for updating

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@auto-submit auto-submit bot merged commit 379f00c into flutter:main Feb 14, 2025
12 checks passed
@bartekpacia
Copy link
Member

wow this is awesome:)

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.

7 participants