-
Notifications
You must be signed in to change notification settings - Fork 2
Bad tests #9
Bad tests #9
Conversation
|
A look, please? |
|
Might still be broken. |
|
Should be good to go. Please review/merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename test class? ;]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see -- you recommended the change because this is no longer a subclass of IntegrationTestCase.
I don't think I should. This is still an integration test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, had a double check - it's integration test. But, we never used integration in test class :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what would you call it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestPageDetail seems fine. Or perhaps we should indicate test types in their names? 🐸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a TestPageDetail. That is the test above.
|
@ian-foote @kevinetienne wanna look as discussed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you ever need to use PageHandler().view? I think this might not work as expected if you do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do. But I think I'm one step ahead of you. ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you might be able to avoid a metaclass by using a classmethod. (For example in incuna-test-utils.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the metaclass makes for a cleaner API, though. One just uses .view, and gets on with it. Perhaps I will change my mind when it comes to adding the management interface.
|
Docstrings. |
For tests?! |
|
Yes. Just a quick summary of what you're testing. |
|
Okay. I'll do the tests from this PR, and I have added #11 for the other tests in the project. |
Docstrings are for code |
|
Do you mind if I do the docstrings in another PR? This one will get too overwhelming with them in, I think. |
|
Thanks |
Bad testing lead to code that wasn't correctly integrated.