Closed Bug 1352023 Opened 7 years ago Closed 7 years ago

Update module names in marionette_driver documentation

Categories

(Remote Protocol :: Marionette, enhancement, P4)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mozilla, Assigned: mozilla, Mentored)

Details

(Whiteboard: [lang=py])

Attachments

(1 file, 1 obsolete file)

Bug 1352023: Fix broken references / typos in Marionette docs;
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
It seems the modules of Marionette have been moved around, but the documentation on http://marionette-client.readthedocs.io/en/latest/index.html wasn't updated to reflect this (except for the "Session Management" section on the "Getting Started" page): Spread across all sites of the documentation there are several occurrences like
> from marionette import <something>
which won't work any more.

This was discovered when trying to follow the Interactive Tutorial.
Priority: -- → P4
Thank you for the bug report Ignaz! Would you be interested to get this updated?
Flags: needinfo?(mozilla)
Sure, I can prepare a patch next week. It should be carefully reviewed though to make sure I'm using the framework as intended.
Flags: needinfo?(mozilla)
(In reply to Ignaz Forster from comment #2)
> Sure, I can prepare a patch next week. It should be carefully reviewed
> though to make sure I'm using the framework as intended.

Sure. I can do it once the patch has been uploaded. Just flag me as reviewer. Thanks.
Mentor: hskupin
Whiteboard: [lang=py]
The attached patch updates the module imports in code examples (and adds them where missing) and fixes broken references to the API documentation.

Some parts of the documentation were referencing methods of the "Alert" and "Timeouts" classes, however they weren't included in the documentation; I've thus added them on the API Reference page. (Note: The API Reference is still leaving out classes like "SelectionManager" or "GeckoInstance", but those seeem to be intentionally left out?)

While at it I also fixed a few syntax, formatting and spelling mistakes. I'm not really happy with the long line in expected.py, but as it's not the only long line in the documentation's examples I thought it should be acceptable.
Attachment #8857931 - Flags: review?(hskupin)
Thank you for the patch Ignaz. I had a quick sneak into it and generally it looks fine. It's also great to see that you fixed all the other forgotten parts! 

I wonder if you would be able to submit the patch to mozreview, instead of just uploading the file to Bugzilla? It would help me a lot with the review process. Documentation in how to set it up can be found here:

https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#mozreview-commits

Thanks!
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mozilla)
Done.
Flags: needinfo?(mozilla)
Comment on attachment 8862622 [details]
Bug 1352023: Fix broken references / typos in Marionette docs;

https://reviewboard.mozilla.org/r/134470/#review139594

Very nice additions! I haven't checked the building yet and if all is correct. I want to do it once the comments I added inline have been addressed. Thanks a lot!

::: testing/marionette/client/docs/advanced/actions.rst:38
(Diff revision 1)
>  For example a user may be dragging one finger while tapping another. This is
>  where :class:`MultiActions` come in. MultiActions are simply a way of combining
>  two or more actions together and performing them all at the same time::
>  
> +    from marionette_driver.marionette import Actions
> +    from marionette_driver.marionette import MultiActions

Just append `MultiActions` to the first line.

::: testing/marionette/client/docs/advanced/findelement.rst:23
(Diff revision 1)
>  Search Strategies
>  -----------------
>  
>  Search strategies are defined in the :class:`By` class::
>  
> -    from marionette import By
> +    from marionette_driver.by import By

Just leave `marionette_driver`.

::: testing/marionette/client/docs/advanced/stale.rst:55
(Diff revision 1)
>  caveats of ``time.sleep(n)``. It will return immediately once the provided
>  condition evaluates to true.
>  
>  To avoid the race condition in the above example, one could do::
>  
> +    from marionette_driver.wait import Wait

`Wait` is also available via `marionette_driver` directly.

::: testing/marionette/client/docs/interactive.rst:48
(Diff revision 1)
>  
>  You can even find an element and click on it. Let's say you want to get
>  the first link:
>  
>  .. parsed-literal::
> -   from marionette import By
> +   from marionette_driver.by import By

Leave the scope as before and just do the rename.

::: testing/marionette/client/marionette_driver/expected.py:168
(Diff revision 1)
>      has a height and width that is greater than 0 pixels.
>  
>      Stale elements, meaning elements that have been detached from the
>      DOM of the current context are treated as not being displayed,
>      meaning this expectation is not analogous to the behaviour of
> -    calling `is_displayed()` on an `HTMLElement`.
> +    calling `is_displayed()` on an ``HTMLElement``.

Can we make this a :func:?

::: testing/marionette/client/marionette_driver/expected.py:212
(Diff revision 1)
>      has a height and width that is greater than 0 pixels.
>  
>      Stale elements, meaning elements that have been detached fom the
>      DOM of the current context are treated as not being displayed,
>      meaning this expectation is not analogous to the behaviour of
> -    calling `is_displayed()` on an `HTMLElement`.
> +    calling `is_displayed()` on an ``HTMLElement``.

Same here.

::: testing/marionette/client/marionette_driver/marionette.py:56
(Diff revision 1)
>  
>      def find_elements(self, method, target):
>          """Returns a list of all ``HTMLElement`` instances that match the
>          specified method and target in the current context.
>  
> -        For more details on this function, see the find_elements method
> +        For more details on this function, see the `find_elements` method

Same here for :func:.

::: testing/marionette/client/marionette_driver/marionette.py:1349
(Diff revision 1)
>  
>      def delete_session(self, send_request=True, reset_session_id=False):
>          """Close the current session and disconnect from the server.
>  
>          :param send_request: Optional, if `True` a request to close the session on
> -            the server side will be send. Use `False` in case of eg. in_app restart()
> +            the server side will be sent. Use `False` in case of eg. in_app restart()

Mind adding :func: references here too to all of the mentioned methods?

::: testing/marionette/client/marionette_driver/marionette.py:1918
(Diff revision 1)
>                  "debug_script": debug_script}
>          rv = self._send_message("executeAsyncScript", body, key="value")
>          return self._from_json(rv)
>  
>      def find_element(self, method, target, id=None):
> -        """Returns an HTMLElement instances that matches the specified
> +        """Returns an ``HTMLElement`` instances that matches the specified

Please fix the typo too: s/instances/instance/
Attachment #8862622 - Flags: review?(hskupin) → review-
Attachment #8857931 - Attachment is obsolete: true
Attachment #8857931 - Flags: review?(hskupin)
Ignaz, do you have an update for us? Will you be able to finish this patch? If not please let me know and I can do it. Thanks.
Flags: needinfo?(mozilla)
Hi Henrik,

it's on my TODO list, but I may not be able to finish it 'til the end of the month. I certainly won't complain when you fix it first ;-)

One note about using :func: in marionette.py: Only the .rst files are currently using clickable references. The API documentation isn't, so  left it as it is (my original goal was to only fix broken references and imports...)
Flags: needinfo?(mozilla)
Ok, that sounds fine. I think that we can still wait a couple more days given that it's broken for a long time now. Thank you!
The new version of the patch is adding references for all functions and classes in the source files as suggested. To avoid having to reference each method with it's full name even within the same class I added currentmodule directives to reference.rst before autogenerating the specific classes' API documentation.
Comment on attachment 8862622 [details]
Bug 1352023: Fix broken references / typos in Marionette docs;

https://reviewboard.mozilla.org/r/134470/#review150270

I checked all the changes and those look great. Thank you so much for doing that, Ignaz! 

Before I land the patch, I will simply submit a try build to ensure nothing breaks and the linter is happy. If all is green, we can get the patch merged.
Attachment #8862622 - Flags: review?(hskupin) → review+
Comment on attachment 8862622 [details]
Bug 1352023: Fix broken references / typos in Marionette docs;

https://reviewboard.mozilla.org/r/134470/#review150604

::: testing/marionette/client/marionette_driver/expected.py:31
(Diff revision 2)
>  
>          el = Wait(marionette).until(expected.element_present(By.ID, "foo"))
>  
>      Or by using a function/lambda returning an element::
>  
> -        el = Wait(marionette).\
> +        el = Wait(marionette).until(expected.element_present(lambda m: m.find_element(By.ID, "foo")))

As the linter shows those lines are too long:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93606287f5f7&selectedJob=104956582

So please revert the changes which you did here, or use something like:

```
el = Wait(marionette).until(
    expected...)
```
Reverting the changes for the four lines completely is no option, as they are currently broken (see e.g. http://marionette-client.readthedocs.io/en/latest/reference.html#marionette_driver.expected.element_not_present).
I've reverted the changes and only removed the '\' now - this should make the linter happy and generate the documentation correctly.
Comment on attachment 8862622 [details]
Bug 1352023: Fix broken references / typos in Marionette docs;

https://reviewboard.mozilla.org/r/134470/#review151156

::: testing/marionette/client/marionette_driver/expected.py:32
(Diff revisions 2 - 3)
>          el = Wait(marionette).until(expected.element_present(By.ID, "foo"))
>  
>      Or by using a function/lambda returning an element::
>  
> -        el = Wait(marionette).until(expected.element_present(lambda m: m.find_element(By.ID, "foo")))
> +        el = Wait(marionette).
> +                until(expected.element_present(lambda m: m.find_element(By.ID, "foo")))

The syntax is still wrong because you cannot simply move until to the next line. The linter might not give a failure because this is inside a comment, but given that you touch that part fix it correctly.

So please use this style:

```
el = Wait(self.marionette).until(
    expected.element_present(lambda m: m.find_element(By.ID, "foo")))
```
Comment on attachment 8862622 [details]
Bug 1352023: Fix broken references / typos in Marionette docs;

https://reviewboard.mozilla.org/r/134470/#review151156

> The syntax is still wrong because you cannot simply move until to the next line. The linter might not give a failure because this is inside a comment, but given that you touch that part fix it correctly.
> 
> So please use this style:
> 
> ```
> el = Wait(self.marionette).until(
>     expected.element_present(lambda m: m.find_element(By.ID, "foo")))
> ```

Perfect! Thank you so much for all the work! I'm going to push this change now.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s bcc87b5ec0b7 -d 6d3603a87085: rebasing 400975:bcc87b5ec0b7 "Bug 1352023: Fix broken references / typos in Marionette docs; r=whimboo" (tip)
merging testing/marionette/client/marionette_driver/marionette.py
warning: conflicts while merging testing/marionette/client/marionette_driver/marionette.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
It looks like the code removal on bug 1368674 caused merge conflicts. Ignaz, can you please rebase your changes against latest mozilla-central?
Flags: needinfo?(mozilla)
Sorry for the delay, it took some time until I found out how to properly rebase in Mercurial. The changeset should apply again now.
Flags: needinfo?(mozilla)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc1837d38448
Fix broken references / typos in Marionette docs; r=whimboo
Thanks a lot! I got it landed.
https://hg.mozilla.org/mozilla-central/rev/dc1837d38448
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: