packages icon indicating copy to clipboard operation
packages copied to clipboard

[go_router] Activate leak testing

Open ValentinVignal opened this issue 1 year ago • 22 comments

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ValentinVignal avatar Aug 29 '24 21:08 ValentinVignal

cc @polina-c

ValentinVignal avatar Aug 29 '24 21:08 ValentinVignal

I didn't modify the version number and the CHANGELOG, should I do it or is it not necessary for this?

ValentinVignal avatar Aug 29 '24 21:08 ValentinVignal

Thanks for pushing it! Looks good to me. Will let @chunhtai to stamp.

polina-c avatar Aug 30 '24 01:08 polina-c

override reason: test only.

chunhtai avatar Sep 05 '24 21:09 chunhtai

I am not sure what happen to the ci, can you try to rebase and see if that passes the ci.

chunhtai avatar Sep 05 '24 21:09 chunhtai

looks like the test was trying to run flutter test --color --platform=chrome --web-renderer=canvaskit" in /b/s/w/ir/x/w/packages/packages/go_router

chunhtai avatar Sep 06 '24 18:09 chunhtai

It looks like the signature needs to be

Future<void> testExecutable(FutureOr<void> Function() testMain)

@polina-c do you want me to make a PR to update https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md ?

ValentinVignal avatar Sep 09 '24 09:09 ValentinVignal

It looks like the signature needs to be

Future<void> testExecutable(FutureOr<void> Function() testMain)

@polina-c do you want me to make a PR to update https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md ?

Hm, interesting. Yes, please. Thank you!

polina-c avatar Sep 09 '24 12:09 polina-c

I updated the documentation in https://github.com/dart-lang/leak_tracker/pull/245

ValentinVignal avatar Sep 09 '24 15:09 ValentinVignal

looks like here is a ci failure https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8737284438128899377/+/u/Run_package_tests/pubspec_validation/stdout

chunhtai avatar Sep 12 '24 21:09 chunhtai

Hm. Leak tracker is not recognized as local dependency, while it is part of Flutter.

polina-c avatar Sep 13 '24 01:09 polina-c

Leak tracker is not recognized as local dependency, while it is part of Flutter.

It isn't part of the Flutter SDK, otherwise the dependency would be an SDK dependency and the repo tooling wouldn't flag it. Local means "in this repository", which it also isn't. It's just a random package from pub as far as the repo tooling is concerned. (I'd be happy to take a PR that makes the check cross-reference by querying the pub.dev API for any package that's not SDK and not local, and allowing anything with a Dart or Flutter official publisher.)

Thus https://github.com/flutter/packages/pull/7546/files#r1757634961

stuartmorgan-g avatar Sep 13 '24 11:09 stuartmorgan-g

Thank you for the insights.

https://github.com/flutter/packages/commit/90ee2ca1743ad333e64a261e71620fba2ebcdac5 made the ci pass

ValentinVignal avatar Sep 13 '24 23:09 ValentinVignal

@chunhtai I see you added the label "waiting for tree to go green"? Is there a bot that will automatically merge this PR once it is ready (like the flutter repository)? Or can I merge it by hand if all the checks are successful?

ValentinVignal avatar Sep 20 '24 16:09 ValentinVignal

@chunhtai I see you added the label "waiting for tree to go green"? Is there a bot that will automatically merge this PR once it is ready (like the flutter repository)? Or can I merge it by hand if all the checks are successful?

I am not sure why it doesn't trigger auto merge, let me update the branch and see if it picks up this time

chunhtai avatar Sep 20 '24 16:09 chunhtai

@chunhtai packages/packages/pointer_interceptor/ tests keep timing out. Is it from my changes? It looks unrelated to me

ValentinVignal avatar Sep 25 '24 01:09 ValentinVignal

When you run the tests, is tree red or green: https://flutter-dashboard.appspot.com/#/build If red, wait for it to be green and then rerun. If green, it may be leak tracker effect. Leak tracker may need to be turned off for these tests.

polina-c avatar Sep 28 '24 16:09 polina-c

I'm super confused. The failing package is packages/video_player/video_player_android, packages/video_player/video_player_android/example/integration_test/video_player_test.dart.

image

But is passes on my local. Does anyone have any insight about that?

ValentinVignal avatar Sep 30 '24 11:09 ValentinVignal

I'm super confused. The failing package is packages/video_player/video_player_android, But is passes on my local. Does anyone have any insight about that?

Thank you for persistence!

May be bots fail on different OS than than your local?

When I face such things, I temporarily add debug logs to the failing test, so that I can see what is going on on bots, when they fail. You also can temporarily adjust ci.yaml in the root to reduce number of bots to run, and remove non-needed tests to get the results faster. And you may want to create separate draft PR for the experiments, to avoid noise on this.

polina-c avatar Sep 30 '24 13:09 polina-c

Does this pr touches video_player package? if not, this may just be an outdated branch, can you try rebase?

chunhtai avatar Oct 03 '24 21:10 chunhtai

Does this pr touches video_player package? if not, this may just be an outdated branch, can you try rebase?

No it doesn't, that's why I'm having hard time understanding the issue 😅

ValentinVignal avatar Oct 07 '24 06:10 ValentinVignal

It looks like the failing test is also failing on main. After it is fixed, this PR should be good to go

ValentinVignal avatar Oct 16 '24 09:10 ValentinVignal