dart-lang / test

A library for writing unit tests in Dart.
https://pub.dev/packages/test
494 stars 213 forks source link

Expose current `testRandomizeOrderingSeed` from Configuration #1572

Open kturney opened 3 years ago

kturney commented 3 years ago

When using --test-randomize-ordering-seed=random, I would like to access the current Configuration.testRandomizeOrderingSeed so that I can use the same seed with https://github.com/drager/faker/pull/32.

I didn't see any current way to access the seed from my tests.

I will personally be running my tests via flutter test if that makes a difference as to where I should be creating this issue.

jakemac53 commented 3 years ago

This is the correct place for the feature request :).

In general it is an intentional choice to not expose Configuration (it is considered an internal detail).

Could you explain a bit more the specific use case?

@natebosch have we considered exposing some configuration like this through the TestHandle?

kturney commented 3 years ago

My thinking is

Infrastructure for this already exists for --test-randomize-ordering-seed=random making a random seed and logging it out so that --test-randomize-ordering-seed={seed} can be used to reproduce the test order.

I'd like to be able to reuse that functionality for seeding my random data as well. In my case that would involve obtaining the current seed from... somewhere... and then passing it along to Faker(seed: {seed}).

jakemac53 commented 3 years ago

Ah ok thanks for the clarification, that makes sense 👍. We hadn't really intended this as a general purpose random seed but I can see why it would be useful to use in that way, and have a single thing that all the tools can use and agree on.

Nate is on vacation but I can discuss it with him when he gets back so you probably won't see anything here for a week or so, but we will get back to it.

natebosch commented 3 years ago

The use case is compelling.

@natebosch have we considered exposing some configuration like this through the TestHandle?

I should try to search if any similar requests have come up... It hasn't come up for the test randomization seed, but I can't remember if it has come up for other runner level configuration.

I think we should go ahead and expose this through a static on TestHandle. Should we expose this directly on TestHandle or through another interface for some potential grouping in case we want to expose similar properties in the future? TestHandle.randomizationSeed, or TestHandle.config.randomizationSeed, or something else?

jakemac53 commented 3 years ago

I think probably just exposing it directly as a static on TestHandle would be fine. I don't see there being a ton of other things we want to expose from the configuration.

One thing I was thinking about with regards to the overall UX here is that by default I think there is no randomization, you have to opt into it. That doesn't necessarily map well to the use case here, where that package probably wants there to be randomization by default? Should we provide a seed here even when test randomization is not enabled?

natebosch commented 3 years ago

Should we provide a seed here even when test randomization is not enabled?

I think we should make the value nullable and document that libraries should use some default or disable randomization as appropriate.

Another question is whether we should surface that the value 0 isn't a seed but a request to disable randomization...

jakemac53 commented 3 years ago

Ya probably just documenting the existing behavior/expectations is best. It probably doesn't match up as perfectly with libraries as one might want, but it is very low risk to expose.

jakemac53 commented 3 years ago

One thing about not giving a random seed when test randomization is disabled, is then we lose the logging about what seed the test libraries used. They could do their own printOnError with the seed they used though.

natebosch commented 3 years ago

https://github.com/dart-lang/test/pull/1541 made this a bit harder than it would have been otherwise.

The design for deserializeSuite relies on the fact that a Configuration is available on the zone to avoid passing everything as arguments. Breaking changes to deserializeSuite are possible, but require coordination with flutter. It's not clear to me we'd want this to impact the signature of deserializeSuite and PlatformPlugin.load either to be able to plumb this through.

We do currently pass a SuiteConfiguration through, so we might be able to model it on that class again, but avoid the bugs that were fixed in #1541