Skip to content

Register flutter/plugins analysis phase#66

Closed
amirh wants to merge 2 commits intoflutter:masterfrom
amirh:analyze_plugins
Closed

Register flutter/plugins analysis phase#66
amirh wants to merge 2 commits intoflutter:masterfrom
amirh:analyze_plugins

Conversation

@amirh
Copy link
Contributor

@amirh amirh commented Nov 18, 2020

This runs the Dart analysis phase on all packages in flutter/plugins.

Ideally we would like to run all flutter/plugins tests though it is probably too slow as it is right now for flutter/tests. Still just the analysis phase would have protected us from recent regressions (e.g flutter/flutter#69126 (comment), flutter/flutter#69440 (comment)).

A single repetition of this test takes ~10 minutes locally on my laptop, @Hixie this is above the 5 minute rule, but I'd suggest this to be an exception as flutter/plugins often is the first to detect framework regressions.

I'm also expecting the presubmits to timeout (as they currently run 15 repetitions of all tests), @Hixie are you ok with reducing this number to 4 ? (alternatively run just the flutter/plugins test 4 times and the rest of the tests 15 times?)

@amirh
Copy link
Contributor Author

amirh commented Nov 18, 2020

I added a registry/slow folder for slow tests that will less iterations on presubmit. Waiting for @Hixie's feedback, if we decide to land this we'll have to update the post-submit recipe to run the slow tests as well.

@Hixie
Copy link
Contributor

Hixie commented Nov 18, 2020

It might be best just to make this a dedicated shard in flutter/flutter rather than using flutter/tests. That would let you just run all the tests, too. I don't think a slow/ registry is a good idea because it'll make people think they can use that too, and then we'll have a bunch of slow tests, and that will quickly get out of control.

@amirh
Copy link
Contributor Author

amirh commented Nov 18, 2020

I don't believe the problem would be in flutter/flutter where the tests run once, the problem are presubmits (as we run the test 15 times and have a 1 hour limit).

Even if I split the plugins test to it's own Cirrus shard it won't fit within 1 hour if we run it 15 times. Do you see any other way forward other than allowing some tests to be executed less times in presubmits?

@Hixie
Copy link
Contributor

Hixie commented Nov 18, 2020

If you just make a dedicated flutter/flutter shard (unrelated to flutter/tests) then there's no reason to run the tests 15 times.

@amirh
Copy link
Contributor Author

amirh commented Nov 18, 2020

I see, your point is that if it flakes in post-submit it will be easier to root cause. Right?

@amirh
Copy link
Contributor Author

amirh commented Nov 18, 2020

We'll still need a way to specify to not run it 15 times in flutter/tests presubmits though

@Hixie
Copy link
Contributor

Hixie commented Nov 18, 2020

Sorry I'm not conveying what I mean clearly! I'm suggesting that the plugins tests have nothing to do with flutter/tests or customer_testing at all. We'd be creating a dedicated shard in flutter/flutter, that directly checks out the plugins tests and runs them. There'd be no flutter/tests presubmits to worry about because flutter/tests wouldn't be involved in any way.

@amirh
Copy link
Contributor Author

amirh commented Nov 18, 2020

Makes sense, thanks for the clarification.

@amirh amirh closed this Nov 18, 2020
@amirh amirh deleted the analyze_plugins branch November 18, 2020 23:20
@christopherfujino
Copy link
Contributor

We'd be creating a dedicated shard in flutter/flutter, that directly checks out the plugins tests and runs them. There'd be no flutter/tests presubmits to worry about because flutter/tests wouldn't be involved in any way.

@Hixie As this landed in flutter/flutter#70887, it clones HEAD, which means builds are no longer exactly reproducible, and the release branches will probably break. I think at the minimum the revision should be pinned, customer_testing makes sense as it's an existing framework that we use for declaring external repo dependencies and pinning them.

@Hixie
Copy link
Contributor

Hixie commented Nov 23, 2020

See flutter/flutter#70887 (comment) for a way to fix that.
The problem with using customer_testing is that it's intended for third-party tests that run in a very short time. It's important that we present a simple mechanism for people to contribute their tests; the more subtle we make it, the harder it will be for people to understand our policies and so forth. These tests just don't work in that framework.

@jmagman
Copy link
Member

jmagman commented Nov 23, 2020

The problem with using customer_testing is that it's intended for third-party tests that run in a very short time.

Would we accept a third party plugin tests that ran in a short time to customer_testing? Is there anything that special about 95% popular 1p battery vs 99% popular 3rd party geolocator (randomly selected). Related to discussion about 1p plugins and the capacity of the ecosystems team. Maybe we need to make that simple mechanism for plugin authors to contribute their tests, and try it out on our 1p plugins.

@Hixie
Copy link
Contributor

Hixie commented Nov 24, 2020

We would absolutely accept such tests. The simple mechanism is described in https://github.com/flutter/tests/blob/master/README.md and https://github.com/flutter/tests/blob/master/registry/template.test.

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.

4 participants