Skip to content

Conversation

jmagman
Copy link
Member

@jmagman jmagman commented Dec 19, 2023

Treat engine scenario_app as a test and do not add needs-test comment. Example: flutter/engine#49066 (comment)

Refactor tests, put all the code/test combo checks into one test to reduce boilerplate, otherwise I would have needed to add a new test for the scenario_app check.

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.

@jmagman jmagman self-assigned this Dec 19, 2023
@jmagman jmagman requested a review from keyonghan as a code owner December 19, 2023 21:55
}

bool _isATest(String filename) {
bool _isAFrameworkTest(String filename) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing the _isAFrameworkTest definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

This already existed, I just renamed this from _isATest to _isAFrameworkTest since I was adding _isAnEngineTest and wanted to differentiate them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was looking for this function in my local fork..! Sorry for the noise.

Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

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

LGTM

}

bool _isATest(String filename) {
bool _isAFrameworkTest(String filename) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was looking for this function in my local fork..! Sorry for the noise.

@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App. label Dec 19, 2023
@auto-submit auto-submit bot merged commit 917ac5b into flutter:main Dec 19, 2023
@jmagman jmagman deleted the engine-test-ios-scenario branch December 19, 2023 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants