Closed MrGussio closed 2 years ago
ST-DDT
commented
2 years ago Not sure about this one. Sure tests are nice, but these tests actually just test that the definition is correct. IMO we have to compare the faker definitions with the official definitions first. I expect every change to these definitions to be covered by tests, but not every definition.
MrGussio
commented
2 years ago If we construct the tests according to the right definition, we will automatically also find other mistakes if there are any.
The way I discovered the Costa Rica formatting mistake, is that my tests sometimes randomly failed on the IBAN when validating the generated response with validator.js. Since the same library seems to be used with the tests here, we can cross-validate if the definitions are correct using these tests.
Shinigami92
commented
2 years ago I would like to have generic test for every IBAN countryCode but using a it.each for that.
Then if needed a selection of special IBANs that could somehow also be checked via it.each in a more complex form 🤔
Otherwise: I'm with @ST-DDT https://github.com/faker-js/faker/issues/660#issuecomment-1075740583
ST-DDT
commented
2 years ago Lets use validator.js for the generic test: https://github.com/validatorjs/validator.js/blob/master/src/lib/isIBAN.js#L8
Shinigami92
commented
2 years ago Lets use validator.js for the generic test: https://github.com/validatorjs/validator.js/blob/master/src/lib/isIBAN.js#L8
In https://github.com/faker-js/faker/blob/main/test/finance_iban.spec.ts, only a handful of IBAN tests are implemented. Since the discovery of #646, it seems like a good idea to expand this list to cover all IBANs to check if the generation is actually implemented as expected.