arturmkrtchyan / iban4j

A Java library for generation and validation of the International Bank Account Numbers (IBAN ISO_13616) and Business Identifier Codes (BIC ISO_9362).
http://iban4j.org
Apache License 2.0
271 stars 124 forks source link

Allow setting a seed for Random #91

Closed aSemy closed 8 months ago

aSemy commented 2 years ago

I'm using Kotest property based testing, and this needs a deterministic way of generating random data based on a seed, or existing Random instance.

iban4j can generate random IBANs, but I can't set a seed or pass in a Random instance.

I would like to add an overload for the Iban.random() method that will accept a Random parameter.

I can try making a PR for this, if you agree. Thanks!

hajk1 commented 2 years ago

Yes, of course, you can do it.

hajk1 commented 11 months ago

@aSemy There are two failing tests regarding this issue. I disabled them for now. Can you take a look?

MR-Os1 commented 8 months ago

The testcases for this issue is bad because the random generation depends on the BbanStructure and what order the objects added to our EnumMap. the random mechanism should change if we don't want to change this testcase every time we add new BbanStructure or we have to let go of the testcases for this feature. we can make a random method with country input if the other part of the iban require to be random and this will not get affected by new BbanStructure.

hajk1 commented 8 months ago

@MR-Os1 we might have different test cases for both(using random generation or not). Can you handle this one?

aSemy commented 8 months ago

The point of tests was to verify that the random generation of a single IBAN didn't affect subsequent ones. It's the code that's bad, not the tests 😅.

I've opened a PR #126 that should fix the generation, so that each IBAN is generated using an independent Random. Even if multiple IBANs are generated, a seeded IBAN should only change if the structure of its randomly selected country changes, and subsequently generated IBANs shouldn't change.

126 is just one option, maybe there's a way to improve the tests too?

MR-Os1 commented 8 months ago

this is still going to have side effect on adding new BbanStructure

hajk1 commented 8 months ago

this is still going to have side effect on adding new BbanStructure

So maybe it is worth to create a new ticket regarding some improvement to the procedure