search

tnc-ba / strongTNC

BYOD TNC Database Management Tool
GNU Affero General Public License v3.0
0 stars 0 forks source link

Improved & tested enforcement save view #255

Closed dbrgn closed 10 years ago

dbrgn commented 10 years ago

Found this when grepping for TODOs in the sourcecode.

This raises coverage to 60%.

dbrgn commented 10 years ago

Please review, @d22

d22 commented 10 years ago

Nice test! Good to merge! Similar changes could/should be made in any view. Should we create an issue for this, or file it under 'General Code Health'. (I guess the test file is called test_policy.py because of the app name, right? The name is a little confusing at the moment as no policies are tested there...)

dbrgn commented 10 years ago

Nice test! Good to merge! Similar changes could/should be made in any view. Should we create an issue for this, or file it under 'General Code Health'.

Could be done in combination with the class based views. Or with code health. The tests would also help us greatly when refactoring the views.

Regarding the test itself, I wonder whether we should change the parametrization to a regular test with a loop, as a failure in one of the values results in many other failures (the tests are not independent).

(I guess the test file is called test_policy.py because of the app name, right? The name is a little confusing at the moment as no policies are tested there...)

Yep.

d22 commented 10 years ago

Regarding the test itself, I wonder whether we should change the parametrization to a regular test with a loop, as a failure in one of the values results in many other failures (the tests are not independent).

I'm not sure if I understand you correctly. With parametrize we have independet tests, I'd say this is what we want, or isn't it?

I'd say merge as it is now!

dbrgn commented 10 years ago

Only if the tests are independent. See new commit, I think this is better.

d22 commented 10 years ago

:+1: Good to merge!