Incorrect tests & missing test in rect_test

[bilhox]

I'm opening this issue as I don't want to work on it for the moment.

I have noticed many errors when I tried to implement rel_center :

• test_inflate_ip__larger does not check what it is supposed to check (duplicate code of test_inflate_ip__smaller)
• test_inflate_ip__smaller's docstring is incorrect
• each attributes are not checked with literal values if they have the right values. You might think calculating it based on other attributes garantie that the attributes return the right value but this shouldn't be the case ! Not for tests at least ! This is why I would like to see a test that checks if the attributes return the right value based on literal values.

[avpai-dinosaur]

Hi @bilhox,

I'm happy to work on this issue. I see the problem with inflate_ip__smaller's docstring. Also the fact that test_inflage_ip__larger isn't actually making the rect larger.

I agree that the test should have some comparison to literal values. I'm wondering if there is a case where the comparison with attribute succeeds but the comparison to a literal value would fail as that would be the best test case to add.

In any case I can submit a pull request for this.

[avpai-dinosaur]

Actually while working on this I've become less convinced that checks need to be done for literal values. If the previous tests for the attributes work then it seems redundant to add tests for literal values vs just using the attributes. Further comparing to attributes is much more readable.

I'm going to submit the pull request to fix the docstring and test_inflate_ip__larger but hold off on making the cases with literal values. I think it should be two separate commits anyways since many other tests for rect use comparison to attributes (so if there is a reason to compare to literals we should probably add literals to those other tests as well)

Could you expand on why comparison to attributes isn't sufficient?

[bilhox]

Could you expand on why comparison to attributes isn't sufficient?

Basically, it's more a question of ethic, you can't just check if the value is correct using other attributes in a test case. I talked with some python pros about this, they did not really suggest new tests, but what they think it's the most suitable is to actually use literal values which you can predict way before attribute values.

Thank you for taking care of this issue !

[bilhox]

Reopenening it as there is a missing task.

[AntoineMamou]

Hello @bilhox,

We are a team of 6 newcomers to the pygame community. I see that the missing task is about testing each attribute of rect_test with literal values. Could we work on this issue ?

[bilhox]

Yes sure !

[AntoineMamou]

Hi @bilhox,

I made the fix for the test functions : test_inflate__larger, test_inflate__smaller, test_inflate_ip__larger and test_inflate_ip__smaller. Should I push my code now ? And if yes, how to do it correctly ?

There is also the same problem for other functions. Should I correct them too ?

[bilhox]

Hi @AntoineMamou ,

If you find other things that seem wrong for you, don't worry, you can edit it, we'll see if it's necessary when we'll be reviewing your code.