faker icon indicating copy to clipboard operation
faker copied to clipboard

Make science tests more specific

Open ejcheng opened this issue 3 years ago • 5 comments

science.chemicalElement and science.unit both return objects. We should add a test for those two methods to check if the properties of the object returned by a method call correspond with one another.

E.g. if science.unit() returns the object { name: 'meter', symbol: 'kg' }, it would fail the test, because meter and kg do not correspond with one another. However, if the method returns { name: 'meter', symbol: 'm' }, it would pass the test, because m and meter correspond.

ejcheng avatar Jul 24 '22 06:07 ejcheng

IDK, a test doesn't seem the right thing for me. Since these modules only return a random element from the locale definition. By adding a "test" that meter has the symbol m you are only testing that you implemented what you really implemented. You don't add any value by introducing such a test.

If science.unit() really returns { name: 'meter', symbol: 'kg' } I would classify this as a bug that was introduced due to lackluster code review, but I cant reproduce the scenario you described.

xDivisionByZerox avatar Jul 24 '22 12:07 xDivisionByZerox

Are your referring to a specific issue, or do you just want to test the contents of our locale data? Currently we don't test the contents of the locale data (outside of the code review) only the functionality of the implementation. If we would do content checks, we would have to include a spell checker for the other locale data as well.

Could you please explain the intentions behind this proposal some more?

ST-DDT avatar Jul 24 '22 13:07 ST-DDT

IDK, a test doesn't seem the right thing for me. Since these modules only return a random element from the locale definition. By adding a "test" that meter has the symbol m you are only testing that you implemented what you really implemented. You don't add any value by introducing such a test.

If science.unit() really returns { name: 'meter', symbol: 'kg' } I would classify this as a bug that was introduced due to lackluster code review, but I cant reproduce the scenario you described.

That scenario is only theoretical, the definitions are all currently correct and not buggy. These proposed added tests are only to test the off-chance that the definitions were somehow changed and the name and symbol properties no longer match up with each other.

ejcheng avatar Jul 24 '22 14:07 ejcheng

These proposed added tests are only to test the off-chance that the definitions were somehow changed and the name and symbol properties no longer match up with each other.

So basically you want to that meter is always bound to m. However, this check only works for en and is basically a duplication of the actual data. There is no way to test these things unless you duplicate the actual data for the tests, so that you can use it to test them. IMO there is no value in duplicating data.

Or do you have a different approach in mind, that does not require duplicating the actual data?

ST-DDT avatar Jul 25 '22 07:07 ST-DDT

These proposed added tests are only to test the off-chance that the definitions were somehow changed and the name and symbol properties no longer match up with each other.

So basically you want to that meter is always bound to m. However, this check only works for en and is basically a duplication of the actual data. There is no way to test these things unless you duplicate the actual data for the tests, so that you can use it to test them. IMO there is no value in duplicating data.

Or do you have a different approach in mind, that does not require duplicating the actual data?

Hmm I was thinking about using an Object or a Map in the background as a dictionary, setting one of the properties of the object returned by each function as the key and the other property as the value, calling each function multiple times, and then checking if the key is contained within the output and the value in the key-value pair corresponds with the value in the dictionary.

However, this would be complicated and it would duplicate data, as you said. I'm not sure how to go about it without duplicating data.

ejcheng avatar Jul 25 '22 14:07 ejcheng

Closes because, see reasons above.

Shinigami92 avatar Sep 09 '22 10:09 Shinigami92