Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting a seed for Random #91

Closed
aSemy opened this issue Aug 18, 2022 · 7 comments · Fixed by #93 or #126
Closed

Allow setting a seed for Random #91

aSemy opened this issue Aug 18, 2022 · 7 comments · Fixed by #93 or #126

Comments

@aSemy
Copy link
Contributor

aSemy commented Aug 18, 2022

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
Copy link
Collaborator

hajk1 commented Aug 18, 2022

Yes, of course, you can do it.

@hajk1
Copy link
Collaborator

hajk1 commented Oct 2, 2023

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

@hajk1 hajk1 reopened this Oct 2, 2023
@MR-Os1
Copy link
Contributor

MR-Os1 commented Jan 6, 2024

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
Copy link
Collaborator

hajk1 commented Jan 8, 2024

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

@aSemy
Copy link
Contributor Author

aSemy commented Jan 8, 2024

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
Copy link
Contributor

MR-Os1 commented Jan 10, 2024

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

@hajk1
Copy link
Collaborator

hajk1 commented Jan 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants