-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Comments
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. |
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? |
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 ! |
Reopenening it as there is a missing task. |
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 ? |
Yes sure ! |
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 ? |
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. |
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 oftest_inflate_ip__smaller
)test_inflate_ip__smaller
's docstring is incorrectThe text was updated successfully, but these errors were encountered: