Skip to content
This repository was archived by the owner on Jun 13, 2024. It is now read-only.

Conversation

@amanv8060
Copy link
Contributor

Migrate test_benchmarks to null safety

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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.

@required Rect large,
@required Rect small,
@required AxisDirection axisDirection,
bool? _hasSufficientFreeRoom({
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like the call to this handles it potentially being null, so I'd make it non-nullable, and then in line 43 below give it an initial value (either true or false) before the switch statement.

@amanv8060
Copy link
Contributor Author

Can't figure out the failure, meanwhile closing it for anyone else to have a look.

@amanv8060 amanv8060 closed this Aug 29, 2022
@guidezpl
Copy link
Member

Re-opening to see/fix the failures

@guidezpl guidezpl reopened this Aug 30, 2022
@mit-mit
Copy link
Member

mit-mit commented Aug 30, 2022

@guidezpl I can't make sense of this error. Do you understand what's going on?
https://github.com/flutter/gallery/runs/8087094487?check_suite_focus=true

@pennzht
Copy link
Member

pennzht commented Sep 5, 2022

@guidezpl I can't make sense of this error. Do you understand what's going on? https://github.com/flutter/gallery/runs/8087094487?check_suite_focus=true

Taking a look now.

pennzht pushed a commit to pennzht/newfluttergallery that referenced this pull request Sep 5, 2022
@pennzht
Copy link
Member

pennzht commented Sep 5, 2022

Thank you for your message!

I tested the web benchmarks a few times locally, and it worked every time after updating the package versions.

A similar PR #768 passes all checks.

Have you tried re-running the web benchmark checks? Does the failure persist?
It could be possible that this test is flaky, although I haven't observed any flakiness before.

@mit-mit
Copy link
Member

mit-mit commented Sep 5, 2022

I've tried several re-runs over several days. I just did it again, this time selecting to include debug info. But I still don't see anything that helps me understand what's going on:
https://github.com/flutter/gallery/runs/8191884582?check_suite_focus=true

@amanv8060 amanv8060 changed the title refactor: machine migration refactor: migrate test_benchmarks to null safety Sep 5, 2022
@amanv8060 amanv8060 force-pushed the benchmarks_migration branch from 87eea15 to 787a122 Compare September 5, 2022 20:26
@amanv8060 amanv8060 marked this pull request as ready for review September 5, 2022 20:33
@amanv8060
Copy link
Contributor Author

Thanks @pennzht this was something related to versions in lockfile, updating them did the job.
Goldens are failing since : #769 , should they be updated in seperate pr @guidezpl ?

Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

Nice. I'll fix the goldens issue separately (the banner divider changed color) after the change is rolled in

@guidezpl guidezpl linked an issue Sep 6, 2022 that may be closed by this pull request
@guidezpl guidezpl merged commit dd4eb8f into flutter:main Sep 6, 2022
@amanv8060 amanv8060 deleted the benchmarks_migration branch September 6, 2022 08:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Address null safety exclusions in flutter/gallery

4 participants