Code review comment for lp:~elopio/u1-test-utils/page-objects

Revision history for this message
Leo Arias (elopio) wrote :

> Liking the pages approach.
>
> A few questions/comments:
>
> - return_page should be a Page instance? related, a Page class is expected
> here (l.103)?

Yes. We are passing instantiated page objects. At first I tried passing just the classes so the verification could be done when we instantiated them, but it seemed harder to understand. l.103 is a typo left from that experiment, I fixed it.

> - the return_page argument seems to be just for checks? (verifying the user is
> correctly redirected after an action to that page); at first I thought it was
> a way to tell the action where to redirect me after it. I would expect getting
> a page instance as result and maybe the caller check the got object in an
> instance of the right page?

It's a way to make sure we are on the right page, and a way to make it clearer for the caller where I will be after the call.
A page instance as a result is what I like too, but I thought the check could be done on the action that leads to that page instead of leaving the job to every caller. I added to the __init__ a call to assert_page_is_open, and I added it too to all methods that don't instantiate the return page, so there's no need for the caller to do an extra check. Maybe I'm not getting right what you are saying, if you can give me a little code snippet it will be easier.

> - shouldn't l.149 and l.332 be comments instead?

I think not. Why? They are docstrings, and according to pep257 they should use triple double quotes. It really doesn't say anything specific to class attributes, but that's what I understood. I might be wrong.

« Back to merge proposal