-
Notifications
You must be signed in to change notification settings - Fork 361
Fix a skipped test in inspector_service_test (was a false positive) #6942
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
Fix a skipped test in inspector_service_test (was a false positive) #6942
Conversation
|
@kenzieschmoll looks like the tests don't run on latest master branch? |
|
There can be a few days of latency. When our CI starts failing at master because the flutter version caught up, we can land this PR to fix it. Thanks! |
jacob314
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing.
Fyi @goderbauer is the identifier property on Semantics important enough that we want to show it in the debugging output even when it is null?
|
I'm not @goderbauer but since I added Semantics.identifier recently I'll say that it's not. I agree it can be removed when it's null. |
@chunhtai Do you have an opinion here? |
|
I don't think it should show up if it is null, the tooltip probably shouldn't show up as well. I can't see how they may be useful here. @bartekpacia can you adjust the SemanticsProperties.debugFillProperties to give them a default value so it wouldn't show up here? |
) This PR applies [the suggestion made here](flutter/devtools#6942 (comment)).
|
I implemented @chunhtai's suggestion from above in flutter/flutter#140283, I think we now have to wait a few days until a framework roll into devtools. |
|
Will the golden output here need to be modified for your latest Flutter PR? |
|
Yes, after changes from my latest PR both |
| │ identifier: null | ||
| │ tooltip: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't these need to be removed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right.
I was waiting for framework roll.
…ter#140283) This PR applies [the suggestion made here](flutter/devtools#6942 (comment)).
|
@kenzieschmoll The tests are failing, is the approval intentional? BTW I looked briefly at the error message and can't understand why it fails: Logs
|
|
Spoke to @kenzieschmoll about this last week and she said she's gonna take another look at this. |
|
Thanks for the update @goderbauer! |
…ctor_service_test_reenable
…ctor_service_test_reenable
this hash includes flutter/devtools#6942, which unskips an inspector test.

Fixes #6902
Pre-launch Checklist
///).