762126 - Allow to specify the screenshot path via the persisted object (CLI option)

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now we save the screenshots to the mozmill-screenshots subfolder under temporary files. That's not always wanted and for Mozmill Crowd we want to store them in the same folder as the user has specified.

Therefore in our automation scripts we are using the persisted object and a CLI option (--screenshot-path). See bug 614972.

Now that the screenshot module is part of Mozmill we should do the same here.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/81

Pointer to Github pull-request

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 647370 [details]
Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/81

This created the file in the appropriate directory, however I was unable to open the image even if it was created in the temporary path location. There also seems to be some discrepancy with file types - a comment mentions PNG but the file name is JPG.

Comment 3

[Jeff Hammel]

We should probably add the argument to mozmill.create as well

Comment 4

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #3)
> We should probably add the argument to mozmill.create as well

:davehunt, do you mind fixing this first?  Other than that, the patch looks mostly fine.  Should probably be more explicit in the CLI help that the path == a directory

+            path = os.path.abspath(screenshot_path)
+            if not os.path.isdir(path):
+                os.makedirs(path)
+            self.persisted['screenshotPath'] = screenshot_path

I would assume that file.initWithPath(frame.persisted['screenshotPath']); works with relative paths?  

So if you don't pass the screenshot_path, how does one know where the screenshots go?  Should this be added to persisted from the JS side?  Or, alternately, should python create the directory?  Something should print out where the screenshots are.  I'm not sure what the present status is

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jeff Hammel [:jhammel] from comment #4)
> I would assume that file.initWithPath(frame.persisted['screenshotPath']);
> works with relative paths?  

I don't think so. We should make it an absolute path before assigning it to persisted.

> So if you don't pass the screenshot_path, how does one know where the
> screenshots go?  Should this be added to persisted from the JS side?  Or,
> alternately, should python create the directory?  Something should print out
> where the screenshots are.  I'm not sure what the present status is

If no path gets specified we should fallback to the temp folder, which we currently use to save screenshots to. Well, it's a subfolder under temp. But that logic could be part of the python code and would make the JS side cleaner.

Comment 6

[Jeff Hammel]

I'm sorry I didn't address this sooner, this got lost in my inbox

(In reply to Henrik Skupin (:whimboo) from comment #5)
> (In reply to Jeff Hammel [:jhammel] from comment #4)
> > I would assume that file.initWithPath(frame.persisted['screenshotPath']);
> > works with relative paths?  
> 
> I don't think so. We should make it an absolute path before assigning it to
> persisted.
> 
> > So if you don't pass the screenshot_path, how does one know where the
> > screenshots go?  Should this be added to persisted from the JS side?  Or,
> > alternately, should python create the directory?  Something should print out
> > where the screenshots are.  I'm not sure what the present status is
> 
> If no path gets specified we should fallback to the temp folder, which we
> currently use to save screenshots to. Well, it's a subfolder under temp. But
> that logic could be part of the python code and would make the JS side
> cleaner.

I agree on both points.  r-ing for now until these points are addressed.  We should
- specify the absolute path in all cases
- and specify the screenshot path from the python side
- (and preferably print out the path if any screenshots are saved)

I wouldn't mind if the directory creation on non-existence happens on the JS side, saving a tempdir if there are no screenshots.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/88

Pointer to Github pull-request

Comment 8

[Jeff Hammel]

Comment on attachment 662984 [details]
Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/88

This wfm.  Sorry, this got lost in my review queue for awhile

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, there is one thing I would like to see addressed. Looking into the future it would probably helpful to have a screenshot property on the persisted object which can hold more than only the path, e.g. the screenshot format.

So would you mind changing the following?

< self.persisted['screenshotPath'] = screenshot_path
> self.persisted['screenshot']['path'] = screenshot_path

With it we would be totally flexible for upcoming additions and we wouldn't have to change our dashboard once we support screenshots.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 662984 [details]
Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/88

Pull request updated as suggested in comment 9.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 662984 [details]
Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/88

Looks great now for the patch, but I really would like to see a test for this feature too.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 662984 [details]
Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/88

Requesting feedback for my first mutt tests.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 662984 [details]
Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/88

Good start Dave! It looks fine. I have made some more detailed comments on the pull.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 662984 [details]
Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/88

Updated pull request based on feedback

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 662984 [details]
Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/88

Looks good. I have made comments on the pull request. I wonder if we should move the patch over here for the next iteration.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow specifying screenshot path. v2.0

Moved to Bugzilla as suggested. Closed outstanding pull request in github.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 714343 [details] [diff] [review]
Allow specifying screenshot path. v2.0

Review of attachment 714343 [details] [diff] [review]:
-----------------------------------------------------------------

Some comments to address. Otherwise it looks good.

::: mozmill/mozmill/__init__.py
@@ +680,5 @@
>                           help="Specify an event handler given a file PATH "
>                                "and the CLASS in the file")
> +        group.add_option('--screenshots-path',
> +                         dest='screenshots_path',
> +                         default=None,

nit: please kill this line. It's not necessary.

::: mutt/mutt/tests/python/testscreenshotpath.py
@@ +16,5 @@
> +          controller = mozmill.getBrowserController();
> +        }
> +
> +        let test = function() {
> +          var backButton = new elementslib.ID(controller.window.document, 'back-button');

Please use mozElement to retrieve elements in Mozmill 2.

@@ +41,5 @@
> +        assert len(screenshots) == 1
> +        screenshot = os.path.join(screenshots_path, '%s.jpg' % screenshot_name)
> +        assert screenshots[0]['filename'] == screenshot
> +        assert os.path.isfile(screenshot)
> +        shutil.rmtree(screenshots_path)

Mind adding this also to tearDown? So we can ensure to delete the folder if the test fails due to whatever reasons.

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow specifying screenshot path. v2.1

Updated according to review feedback.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 721203 [details] [diff] [review]
Allow specifying screenshot path. v2.1

Review of attachment 721203 [details] [diff] [review]:
-----------------------------------------------------------------

One thing to fix. With it my r+ and you can land it.

::: mutt/mutt/tests/python/testscreenshotpath.py
@@ +16,5 @@
> +          controller = mozmill.getBrowserController();
> +        }
> +
> +        let test = function() {
> +          var backButton = new mozelement.ID(controller.window.document, 'back-button');

Can you please do it like the MDN page tells?

https://developer.mozilla.org/en-US/docs/Mozmill/Finding_Mozmill_Elements

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow specifying screenshot path. v2.2

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 727627 [details] [diff] [review]
Allow specifying screenshot path. v2.2

Carrying r+ from previous review.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
https://github.com/mozilla/mozmill/commit/e9054305d777eedae37fa0abc2426e50fb2b58a1