fix: update pydecimal algorithm to ensure left part is not generated with a leading 0

[alexei]

What does this change

This changes the algorithm of pydecimal to ensure the integer part if of the specified length.

What was wrong

As suggested in #1992, it is possible for pydecimal to generate a value with an integer part that's shorter than the specified length.

How this fixes it

The original algorithm generated a list of digits and joined it into an integer. However, with that approach it is possible that one or more leading digits are 0, thus ending up with a shorter value.

For example,

from faker import Faker
fake = Faker()
fake.seed_instance(15)
fake.pydecimal(left_digits=8, right_digits=3)

Generates 08023002 for left number, thus resulting in Decimal("8023002.531"). Since 8023002 is only 7 digits, it does not satisfy the requirement that the resulting value has 8 left digits.

Fixes #1992

[alexei]

I fear the tests I added are only relevant to the original algorithm. Let me know how to go about this.

[fcurella]

The issues with hardcoding a specific seed in tests is that different versions of Python may return a different value. What we usually do is set the seed to 0, and then run the same test for some number of iteration, and hope for the best 🤞🏼

[alexei]

I did try that initially and I gave up after a couple of million iterations. @stefan6419846 suggested to try to find a seed and that did the trick. I also noticed there were other tests that seeded the generator so I figured it was harmless.

However, as I pointed out in my previous comment, the tests are only relevant with the previous technique where digits are generated individually. With this change an integer is generated as a whole, so that bug would no longer exist. I would say it's not worth keeping the tests. Besides, the number of digits is already checked in an earlier test. These two are just to reproduce the bug I reported. What do you think?

[stefan6419846]

My initial proposal with a fixed seed is mostly helpful for doing local debugging on your setup as once you have found a suitable seed, digging through the implementation and analyzing where the issue stems from makes the most sense. This does not necessarily need to hold true as mentioned in the previous comments in this PR for actual unit/integration tests.

[alexei]

There are other tests doing the same:

faker/tests/providers/test_python.py

Lines 444 to 467 in 1dc888b

	def test_min_value_zero_doesnt_return_negative(self):
	Faker.seed("1")
	result = self.fake.pydecimal(left_digits=3, right_digits=2, min_value=0, max_value=999)
	self.assertGreater(result, 0)
	def test_min_value_one_hundred_doesnt_return_negative(self):
	Faker.seed("1")
	result = self.fake.pydecimal(left_digits=3, right_digits=2, min_value=100, max_value=999)
	self.assertGreater(result, 100)
	def test_min_value_minus_one_doesnt_return_positive(self):
	Faker.seed("5")
	result = self.fake.pydecimal(left_digits=3, right_digits=2, min_value=-999, max_value=0)
	self.assertLess(result, 0)
	def test_min_value_minus_one_hundred_doesnt_return_positive(self):
	Faker.seed("5")
	result = self.fake.pydecimal(left_digits=3, right_digits=2, min_value=-999, max_value=-100)
	self.assertLess(result, -100)
	def test_min_value_10_pow_1000_return_greater_number(self):
	Faker.seed("2")
	result = self.fake.pydecimal(min_value=10**1000)
	self.assertGreater(result, 10**1000)

In any case, this is focusing on the wrong question. I think I should drop the tests, as they're not necessary. If you think the tests should stay, then please either guide me so I can rewrite them in a way that fits the codebase, or change them yourselves.

[fcurella]

I wonder if we could mock the random generator on the instance to force it to return a fixed value. I can take a look into it later today

…

On Thu, Feb 15, 2024 at 5:08 AM Alexandru Mărășteanu < ***@***.***> wrote: There are other tests doing the same: https://github.com/joke2k/faker/blob/1dc888b36cbdcd91c31bbcbe71d34267acdd5898/tests/providers/test_python.py#L444-L467 In any case, this is focusing on the wrong question. I think I should drop the tests, as they're not necessary. If you think the tests should stay, then please either guide me so I can rewrite them in a way that fits the codebase, or change them yourselves. — Reply to this email directly, view it on GitHub <#1994 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAV4B3MLTWIUZPMQ3IYD4LYTXUBBAVCNFSM6AAAAABDISWBS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBVHA3DONBZGI> . You are receiving this because you commented.Message ID: ***@***.***>

[fcurella]

ok, I've looked into this and now I have a better understadning of what you're saying. I'd say let's drop the tests

[alexei]

@fcurella thanks for taking a look; I dropped the tests

[fcurella]

Thank you!