Skip to content
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

Remove failed assert. #145

Closed
wants to merge 1 commit into from
Closed

Remove failed assert. #145

wants to merge 1 commit into from

Conversation

polina-c
Copy link
Contributor

@polina-c polina-c requested a review from CoderDake as a code owner September 19, 2023 19:38
Copy link

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

It doesn't seem like you should be removing an assert that is checking that leak tracker is working?

@polina-c
Copy link
Contributor Author

polina-c commented Sep 19, 2023

It doesn't seem like you should be removing an assert that is checking that leak tracker is working?

This assert should never fail because of tracked objects. If it failed, this means there is an algorithmic error. And it seems it failed because of duplicated hash code. Leak tracker expects duplicates, but it seems not very reliably. And this story is not easy to test.

I plan to refactor leak_tracker to stop counting on hashes. But for now I do not want this noise in flutter test failures. There is enough flakiness in flutter itself :)

Makes sense?

Copy link

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

I guess, but seems like a pretty severe issue. if its not happening frequently, you might just want to leave it as is

@polina-c
Copy link
Contributor Author

I guess, but seems like a pretty severe issue. if its not happening frequently, you might just want to leave it as is

Maybe. Lets have this PR ready to merge. I will do some adjustments and maybe we will not need it.
Converted to draft.

@polina-c polina-c marked this pull request as draft September 19, 2023 19:56
@polina-c
Copy link
Contributor Author

I updated leak_tracker to stop identifying objects by hash codes. So, this PR should not be needed.

@polina-c polina-c closed this Sep 20, 2023
@polina-c polina-c deleted the no-assert branch October 18, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants