mtedone / podam

PODAM - POjo DAta Mocker
https://mtedone.github.io/podam
MIT License
326 stars 749 forks source link

Needs ability to set a seed from which all randomness is generated #267

Closed tom-p44 closed 6 years ago

tom-p44 commented 6 years ago

Tests should be repeatable. In unit tests, a seed should be generated and logged, then passed in to be used as a source of the randomness when generating any and all random data. If tests fail on certain random data, the specific seed used for the data can be used to recreate the full state of the unit test.

daivanov commented 6 years ago

Hi,

This is an arguable thing, if random values should be used in tests. But what is not arguable, is one must log it properly, so there will be no need to repeat tests. And with lots of cases like random factors and race conditions repeating is not really an option. Neither it's option with reports from the user.

Thanks, Daniil

daivanov commented 6 years ago

As you always can call ThreadLocalRandom.setSeed(long) I don't think this an issue. Don't you mind if I close this issue?

tom-p44 commented 6 years ago

Ah, if you're saying there is a way to set the seed, then yes, this can be closed. It would still be beneficial, IMO, to explicitly provide a way to set the seed via the API.

mkq commented 1 year ago

I, too, think that tests should be repeatable (and didn't expect it to be "arguable"). I suggest

  1. using a fixed seed by default and
  2. exposing the underlying Random in the API (for those who like tests flaky ;-)).

Calling ThreadLocalRandom.setSeed feels fragile in two ways: It relies on undocumented (I assume) implementation detail, and nowadays with Streams and all kinds of frameworks in the mix, it's not so obvious that calling it in a @BeforeEach method is sufficient (i.e. all Podam generation happens in that thread which executes @BeforeEach).

mkq commented 1 year ago

Oh, now I tried ThreadLocalRandom.setSeed and noticed that it even throws an UnsupportedOperationException. Would you please reopen this issue?

daivanov commented 1 year ago

Hi,

There is no way repeatable tests due to several reasons.

  1. We speak about RandomDataProviderStrategy, so the purpose itself suggests random results. If need something to be deterministic, then you should implement different DataProviderStrategy or optionally make custom TypeManufacturers, which will do something else rather than random generation.
  2. Podam relies on reflection API and this API doesn't guarantee any orders in methods and class definitions. So even using deterministic stream of values to use in the type manufacturers won't make it deterministic.

    Thanks, Daniil

mkq commented 1 year ago

We speak about RandomDataProviderStrategy, so the purpose itself suggests random results.

IMHO, since the standard library calls a Random with a fixed seed a Random, we shouldn't be too picky about the term, and RandomDataProviderStrategy could very well support such a Random.

Podam relies on reflection API and this API doesn't guarantee any orders in methods and class definitions.

Good point. But couldn't Podam do the sorting?

daivanov commented 1 year ago

Hi,

Sorting output of all resuts of reflection API calls is going to hit performance. And RandomDataProviderStrategy is indeed was designed to produce random data for filling the properties. If the goal it to have deterministic property settings, then we need to create another DataProviderStrategy. TypeManufacturer created a value based on AttributeMetadata. Instead of calling random methods there it is possible to create data from the values in AttributeMetadata and use some method to convert this into the desired value. If you want this then please, create a new issue. It is not complex to expose setting seed, but this is not achieving any meaningful goal.

Thanks, Daniil

mkq commented 1 year ago

I am new to Podam, so I don't know what exactly would be achieved with that (and what I would be supposed to write in that feature request, actually). We already have a concrete DataProviderStrategy and the possibility to implement your own. The new intermediate way would be to only write your own TypeManufacturers?

Sounds like too much effort, when all I want is to "fix" RandomDataProviderStrategy.

Thanks, but I think I'm giving up at this point.

But maybe you could change this issue's label to something like "won't fix". "Not An Issue" was probably based on the assumption that you could ThreadLocalRandom.setSeed.

daivanov commented 1 year ago

Hi,

When you say "we have a concrete DataProviderStrategy and the possibility to implement your own". Does it mean your team has it or we means Podam users? DataProviderStrategy and its TypeManufacturers are responsible for creating of all the data injected into the POJOs, so you can change either of these. But I believe it is easier to change TypeManufacturers and just use them in customized DataProviderStrategy or even existing one.

The RandomDataProviderStrategy does exactly what it was designed for, so fixing is not correct term here. I believe you are requesting a new feature, but since everybody talks about random seed, which seems not very relevant for what seems to be a feature: like deterministic POJO instantiation.

Thanks, Daniil

mkq commented 1 year ago

Thanks for the explanation.

By "we already have", I meant Podam users in general. In our team, the consensus (AFAIK) is to accept Podam's random values and then improve Podam-generated objects by invoking plain old setter methods. This is primarily to ensure the data conforms to some business constraints and suits the current test case, so this would be the same regardless of whether Podam creates random or fixed seed pseudo-random values. Maybe this is what should be done in a custom DataProviderStrategy, but most of us just didn't look into Podam's possibilities that much.