Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@akbiggs
Copy link
Contributor

@akbiggs akbiggs commented Jun 3, 2022

We see a lot of complaints about log
spam related to this specific platform
message, and we haven't implemented it for a while,
so I'm turning off this specific error for
now.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@akbiggs
Copy link
Contributor Author

akbiggs commented Jun 3, 2022

Requesting test exemption since the only observable behavior currently is logging which I don't think we should test against.

@akbiggs akbiggs requested a review from naudzghebre June 3, 2022 18:04
@akbiggs
Copy link
Contributor Author

akbiggs commented Jun 3, 2022

@filmil @sanjayc77 FYI

@filmil
Copy link
Contributor

filmil commented Jun 3, 2022

LGTM

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2022

In general we do want to test that there is no log spam, so if there's a way to verify that it would be good to do so.
We have many tests in general that verify that the output is free of log spam for many specific cases, FWIW.

Copy link
Contributor

@naudzghebre naudzghebre left a comment

Choose a reason for hiding this comment

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

LGTM

@akbiggs
Copy link
Contributor Author

akbiggs commented Jun 3, 2022

"We have many tests in general that verify that the output is free of log spam for many specific cases, FWIW."

Sorry @Hixie I'm having trouble finding these tests. :( Do you have an example I can follow?

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2022

I think mostly they're in flutter/flutter, especially tool tests. But in general anywhere you have a test that can generate log spam, it's usually relatively easy to filter the logs through some code that can then check that only acceptable lines are being output.

@chinmaygarde
Copy link
Contributor

Can we land this?

We see a lot of complaints about log
spam related to this specific platform
message, and we haven't implemented it for a while,
so I'm turning off this specific error for
now.
@akbiggs akbiggs changed the title [fuchsia] Stop setCaretRect from log spamming. [fuchsia] Stop unknown TextInput messages from log spamming. Jun 11, 2022
@akbiggs akbiggs changed the title [fuchsia] Stop unknown TextInput messages from log spamming. [fuchsia] Stop unimplemented TextInput from log spamming. Jun 11, 2022
@akbiggs
Copy link
Contributor Author

akbiggs commented Jun 11, 2022

I can't come up with a strategy to test this so I'm going to land it. I accept that the spammy logs may reappear in the future.

@akbiggs akbiggs added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 11, 2022
@fluttergithubbot fluttergithubbot merged commit 219bd24 into flutter:main Jun 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 11, 2022
@akbiggs akbiggs deleted the stop-caret branch June 11, 2022 21:23
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs tests platform-fuchsia waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants