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

Incorrect tests & missing test in rect_test #3096

Open
bilhox opened this issue Sep 9, 2024 · 8 comments · Fixed by #3098 · May be fixed by #3190
Open

Incorrect tests & missing test in rect_test #3096

bilhox opened this issue Sep 9, 2024 · 8 comments · Fixed by #3098 · May be fixed by #3190
Labels
good first issue rect pygame.rect tests tests (module)
Milestone

Comments

@bilhox
Copy link
Contributor

bilhox commented Sep 9, 2024

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

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

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

bilhox commented Sep 11, 2024

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

bilhox commented Sep 13, 2024

Reopenening it as there is a missing task.

@bilhox bilhox reopened this Sep 13, 2024
@AntoineMamou
Copy link

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

bilhox commented Oct 16, 2024

Yes sure !

@bilhox bilhox added this to the 2.5.3 milestone Oct 16, 2024
@bilhox bilhox added needs testing tests tests (module) and removed needs testing labels Oct 16, 2024
@AntoineMamou
Copy link

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

bilhox commented Oct 23, 2024

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.

@bilhox bilhox linked a pull request Oct 23, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue rect pygame.rect tests tests (module)
Projects
None yet
3 participants