Skip to content

Added platform channel integration tests for macos #110606

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 8, 2022
Merged

Added platform channel integration tests for macos #110606

merged 1 commit into from
Sep 8, 2022

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Aug 30, 2022 •
edited
Loading

fixes #110608

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, including Features we expect every widget to implement.
  • 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.

@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Aug 30, 2022
@gaaclarke
Copy link
Member Author

In the current state everything should work but the following tests do not pass:

@gaaclarke gaaclarke force-pushed the enable-channel-tests-macos branch 11 times, most recently from cd6efb7 to 158af5a Compare September 1, 2022 20:46
@gaaclarke gaaclarke marked this pull request as ready for review September 1, 2022 20:52
@gaaclarke gaaclarke requested a review from keyonghan as a code owner September 1, 2022 20:52
@gaaclarke gaaclarke requested a review from cbracken September 1, 2022 20:52
@gaaclarke
Copy link
Member Author

In the current state everything should work but the following tests do not pass:

I disabled those tests for macos for now. After a couple hours of trying to create a simple fix it will take a bit more of investment for a case that people probably don't care much for.

@gaaclarke gaaclarke force-pushed the enable-channel-tests-macos branch from 158af5a to afca3af Compare September 1, 2022 21:21
cbracken
cbracken requested changes Sep 2, 2022
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

lgtm other than adding an issue link to the TODO.

keyonghan
keyonghan reviewed Sep 2, 2022
View reviewed changes
.ci.yaml
@@ -3750,6 +3750,25 @@ targets:
- bin/**
- .ci.yaml

- name: Mac channels_integration_test
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this task expected to run on either x64 or arm64?
/cc @jmagman is working on distinguishing these Mac host only tasks: #109889

Copy link
Member Author

Choose a reason for hiding this comment

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

It's best if it runs on both but not necessary. Otherwise it doesn't matter which one.

@gaaclarke gaaclarke requested a review from cbracken September 2, 2022 16:38
@gaaclarke
Copy link
Member Author

flutter/engine#35888 needs to land first for this to execute correctly. I tried to fix the writeValue: signature quickly but it is a breaking change that will have to be fixed later.

jmagman
jmagman approved these changes Sep 2, 2022
View reviewed changes
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.

cc @a-wallen

LGTM

@cbracken cbracken mentioned this pull request Sep 8, 2022
Closed
5 tasks
a-wallen
a-wallen approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@a-wallen a-wallen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is great, thanks!

@jmagman
Copy link
Member

jmagman commented Sep 8, 2022

@gaaclarke can you fix up testowners?

@cbracken
Copy link
Member

cbracken commented Sep 8, 2022

(he's on it -- talked over chat when I kicked the bot)

@gaaclarke gaaclarke force-pushed the enable-channel-tests-macos branch from 76fe003 to f5bca55 Compare September 8, 2022 20:11
@gaaclarke gaaclarke force-pushed the enable-channel-tests-macos branch from f5bca55 to 39be5e2 Compare September 8, 2022 20:12
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 8, 2022

auto label is removed for flutter/flutter, pr: 110606, due to - The status or check suite Linux tool_tests_commands has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2022
@auto-submit auto-submit bot merged commit d2434a2 into flutter:master Sep 8, 2022
This was referenced Sep 9, 2022
Merged
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2022
@engine-flutter-autoroll
d2434a2 Added platform channel integration tests for macos (flutter/f…
99a9faf 
…lutter#110606)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 9, 2022
@engine-flutter-autoroll
d2434a2 Added platform channel integration tests for macos (flutter/f…
1f01f4c 
…lutter#110606)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 9, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 9, 2022
@engine-flutter-autoroll
d2434a2 Added platform channel integration tests for macos (flutter/f…
c9c44b3 
…lutter#110606)
@gaaclarke gaaclarke mentioned this pull request Sep 9, 2022
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbracken cbracken cbracken approved these changes

@jmagman jmagman jmagman approved these changes

+2 more reviewers

@keyonghan keyonghan keyonghan left review comments

@a-wallen a-wallen a-wallen approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Platform Channel integration tests don't run for MacOS
5 participants
@gaaclarke @jmagman @cbracken @a-wallen @keyonghan