search

okfn-brasil / rosie

🤖 Python application responsible for Serenata de Amor's intelligence
409 stars 60 forks source link

Simplified the CPF testing suite #97

Open luizberti opened 7 years ago

luizberti commented 7 years ago

Current test structure seemed to use the test's title as a message descriptor rather than the message argument in the assertion call. Tests now are closed up on a per-feature basis, make proper use of the specialised assertions in the unittest module, and use the message field for descriptions.

@lipemorais Does this more or less share your view on how the tests should be refactored?

Relevant #50

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.02%) to 98.023% when pulling f96e2e815c7b9fb3cf0b51f4751d00194b0ebaa9 on luizberti:refactor-cpf-testsuite into c599dc73298fdc5c1ceb4077fa1553c75358e90c on datasciencebr:master.

lipemorais commented 7 years ago

Personally I don't like it. The way the test is written will show possible errors in all the 9 assertions. The way this PR suggests it to be will block remaining assertions after one single fail.

I agree with @cuducos here.

luizberti commented 7 years ago

Aahh, totally forgot unittest stopped the test after a single assertion failed. It's quite the shame, I really think feature-focused test suites are much more readable. That aside I think most of the criticism made here, even though it regards issues that were already present in the code, is worth looking into for this PR.

@cuducos The for loop is prohibitive due to the way the test is structured. I think I have a neat way to make that test work out, which would essentially be making the fixtures mostly self-documenting. I'll make a commit regarding this shortly...

@lipemorais I noticed that too, I think the setUpClass is a nice idea, will be adding to the PR shortly.

cuducos commented 7 years ago

essentially be making the fixtures mostly self-documenting. I'll make a commit regarding this shortly...

I like that! @rennerocha have opened #78 and #79 in that sense, have you checked?

luizberti commented 7 years ago

I hadn't seen those but that is almost exactly what I did lol, just have to tweak some more things and I will be pushing them here

rennerocha commented 7 years ago

In #78 we have several test cases and new scenarios will be included in future, so I believe the use of a separate fixture file as it is now, more appropriate.

However in CPF tests, it will be very unlikely to have new scenarios, and as we have only 9 tests, I would prefer to include everything in the same test file, removing the need of a separate fixture file making reading the test easier.

For this test, in my opinion we should remove the fixture file and go with something like #79. What do you think?

luizberti commented 7 years ago

@cuducos Yeah I still haven't finished this one. I'm currently moving apartments and haven't had time to finish this, once I push some meaningful progress I'll make sure to ping you.