Move out JS code into separate test files to get rid of make_test() methods (no cleanup necessary)

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: whimboo, Assigned: cosmith)

Tracking

unspecified
Bug Flags:
in-testsuite +

Details

(Whiteboard: [mentor=whimboo][lang=py][good first bug][mozmill-2.1], URL)

Attachments

(1 attachment, 12 obsolete attachments)

20.38 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Some of our Mutt tests for Mozmill have to setup stuff for a test like creating a temporary js test file. That should be handled by setUp and not within the real test method. The same applies for tearDown where the temporarily file has to be removed.

There might be also other code we should move out from the real test method.
This sounds like a lot of duplicated code. If this setUp and tearDown turns out to be common across all tests then I would suggest a parent test case class that includes these methods.
(Reporter)

Comment 2

5 years ago
Not all py tests need an example Mozmill JS test. Also tests are different and would have to be customized in the individual test files. So I don't see how to centralize that, except getting rid of this idea and storing the js test files already in the repo.
I have a suggestion, not sure how feasible it might be though.
Maybe we could create a parent class with a constructor with something like optjsfile as an optional parameter to which we can pass the path to the jstest file. If this parameter exits we can do the initialization for it the accordingly, else just the normal setup and teardown initialization/cleanup should suffice.

Any obvious pros/cons?
Maybe this is unnecessary optimisation, but I'm always cautious when I see duplicated code across tests. I'm happy with whatever solution here.
(Reporter)

Comment 5

5 years ago
Not sure if Jeff has an opinion but lets ask him! :)

Comment 6

5 years ago
I don't know if Jeff has an opinion either, being out of the loop of Mozmill for so long.  I guess my hand-wavy opinion is to avoid code duplication in general
(Reporter)

Comment 7

5 years ago
So I'm still behind the idea to put all the autogenerated js code into real files and stick them under a sub folder of the Python tests. That would allow us to share js code between py tests and we don't have to take care of cleaning up the files our tests have created. Does that sound fine?

Comment 8

5 years ago
Working on a patch for this bug. Will pull-request the github repo as soon as it is finished.
(Reporter)

Comment 9

5 years ago
Thanks theod! Instead of a pull request please attach the patch to this bug if possible. That's our preferred way in handling patches.
Assignee: nobody → theod.moz
Status: NEW → ASSIGNED
(Reporter)

Comment 10

5 years ago
Adjusting summary given the conversation we had on IRC and my last comment.
Summary: Enhance those Mutt test with setUp and tearDown methods which have to prepare and cleanup tests → Move out JS code into separate test files to get rid of make_test() methods (no cleanup necessary)

Comment 11

5 years ago
Created attachment 720963 [details] [diff] [review]
Patch that puts JS code embedded in python Mutt test into separate JS files

Here's the patch. Tell me if anything is wrong with it.
Comment on attachment 720963 [details] [diff] [review]
Patch that puts JS code embedded in python Mutt test into separate JS files

Please request feedback or review when attaching patches. Thanks. :)
Attachment #720963 - Flags: review?(hskupin)

Comment 13

5 years ago
Not used to bugzilla yet, where do you find that "review" flag?

Comment 14

5 years ago
Nevermind, I found it on the attachment submit page. Sure I will set it next time.
(Reporter)

Comment 15

5 years ago
Comment on attachment 720963 [details] [diff] [review]
Patch that puts JS code embedded in python Mutt test into separate JS files

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

Thank you for the patch theod. I have a couple of things you should check. I put them all inline.

::: mutt/mutt/tests/python/js-tests/test_persisted.js
@@ +7,5 @@
> +    persisted.fleem = 2;
> +    persisted.number += 1;
> +
> +    delete persisted.foo;
> +}
\ No newline at end of file

Can we name that file `test_persisted_without_stop.js`?

::: mutt/mutt/tests/python/js-tests/test_persisted2.js
@@ +7,5 @@
> +    persisted.fleem = 2;
> +    persisted.number += 1;
> +
> +    delete persisted.foo;
> +    controller.stopApplication();

Can we name that file `test_persisted_with_stop.js`?

::: mutt/mutt/tests/python/test_loggerlistener.py
@@ +9,5 @@
>  
>  class ModuleTest(unittest.TestCase):
> +    relpath = os.path.join("js-tests", "new_empty_function.js")
> +    testpath = os.path.join(os.path.dirname(os.path.abspath(__file__)),
> +                            relpath)

Please move those lines into the test itself. We don't need them sticked onto the test object.

Further `new_empty_function.js` is missing the code which should have been moved out. I would suggest we name the file `empty_test.js`.

::: mutt/mutt/tests/python/testapi.py
@@ +15,4 @@
>  
>      def test_api(self):
>          passes = 1
> +        

nit: trailing white-spacing

::: mutt/mutt/tests/python/testprofilepath.py
@@ +16,5 @@
>  
> +    relpath = os.path.join("js-tests", "new_empty_function.js")
> +    testpath = os.path.join(os.path.dirname(os.path.abspath(__file__)),
> +                            relpath)
> +    dummy_file_name = "test_dummy.js"

I think that's a left-over. The dummy test is new_empty_function.js.

@@ +19,5 @@
> +                            relpath)
> +    dummy_file_name = "test_dummy.js"
> +
> +    def setUp(self):
> +        self.tempdir = tempfile.mkdtemp()

Can we rename tempdir to profile_dir? The current name is confusing because it should be used when you want to access the temporary folder directly.

@@ +31,1 @@
>                                   restore=False)

Please move this line into the setUp method right before copying the dummy test.

@@ +39,2 @@
>                                    '--profile=testprofilepath'],
> +                                 cwd=self.tempdir)

nit: would you mind to fix the white-space issue?
Attachment #720963 - Flags: review?(hskupin) → review-

Comment 16

5 years ago
Hi :whimboo,

Sorry for the delay, I was not receiving bugzilla notifications... 

For the names of the files, I will change them, no problem.

About : 


> ::: mutt/mutt/tests/python/test_loggerlistener.py
> @@ +9,5 @@
> >  
> >  class ModuleTest(unittest.TestCase):
> > +    relpath = os.path.join("js-tests", "new_empty_function.js")
> > +    testpath = os.path.join(os.path.dirname(os.path.abspath(__file__)),
> > +                            relpath)
> 
> Please move those lines into the test itself. We don't need them sticked onto the test object.

Sure, I think I placed them there in reference to what was already done in another Python test file but I cannot find it right now so I guess I just misread it.

I will make those changes and upload another patch ASAP. Thanks for the review.
(Reporter)

Comment 17

5 years ago
Thanks Teod. Whenever you have the time please upload the patch and I will make sure to get it reviewed in time.

Comment 18

5 years ago
Comment on attachment 720963 [details] [diff] [review]
Patch that puts JS code embedded in python Mutt test into separate JS files

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

::: mutt/mutt/tests/python/test_loggerlistener.py
@@ +9,5 @@
>  
>  class ModuleTest(unittest.TestCase):
> +    relpath = os.path.join("js-tests", "new_empty_function.js")
> +    testpath = os.path.join(os.path.dirname(os.path.abspath(__file__)),
> +                            relpath)

What do you mean by 

> `new_empty_function.js` is missing the code which should have been moved out.

The original JS code was :

> var test_something = function() {}

And that's exactly what's inside the `new_empty_function.js` file...

Comment 19

5 years ago
(Damn, I have to get used to that bugzilla thing, sorry for the double-comment, then)
I've fixed the patch but currently have issues re-running the tests. I'll upload the new patch version with the required fixes as soon as I can check that all tests still pass, I'll look at that just tomorrow.
(Reporter)

Comment 20

5 years ago
Theod, I haven't heard from you for about a month. Will you be able to finalize the patch soon? Thanks.
Flags: needinfo?(theod.moz)

Comment 21

5 years ago
whimboo: In fact, the patch has been fixed since a while. But I cannot get the mutt tests running on my setup. They all fails (even without the changes). So, as I was not able to assert the tests were passing, I did not want to send the new version of the patch. I should certainly reinstall all my dev env., I did not find the time to do it recently... I will try to do it ASAP.
Flags: needinfo?(theod.moz)
(Reporter)

Comment 22

5 years ago
Thanks! Let me know if you still encounter issues and we can try to get those fixed on IRC if necessary.
(Reporter)

Comment 23

5 years ago
theod, any news regarding the current state of your dev environment? Have you had the time to check that? Or would we have to find someone else to finish this patch? Thanks.
Flags: needinfo?(theod.moz)
(Reporter)

Comment 24

5 years ago
Given that we haven't gotten a reply anymore from theod, I assume he is no longer working on this bug. I will put it back into the pool.
Assignee: theod.moz → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(theod.moz)
(Assignee)

Comment 25

5 years ago
Hi everyone, I'm looking for a first bug to start contributing and this one seems like a good candidate :)

I didn't find anyone on IRC tonight so I figured I might as well post here directly.

I started applying/updating Theod's last patch and everything seems to work fine, however I noticed that some of the (newer?) tests have a do_test() method that appears to do fix this bug. Should I update the existing code to this new way of doing things (namely, adding a do_test method and moving the JS to python/js-modules)?

Let me know what you think!
I recommend using the "need more information" flag when asking questions.
Flags: needinfo?(hskupin)
(Reporter)

Comment 27

5 years ago
Hello Corentin! It's great to see your interest in working on this bug. It's indeed an easier one to get started with. All what you said makes sense, and I agree with your proposal to update to the new format. That's what we established in the past couple of weeks when adding new tests.
Assignee: nobody → corentin.smith
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
(Assignee)

Comment 28

5 years ago
Created attachment 8338195 [details] [diff] [review]
Ok first attempt at submitting a patch, not sure if this is what I'm supposed to do here.
Attachment #8338195 - Flags: review?(hskupin)
(Assignee)

Comment 29

5 years ago
Looks like it worked, I wasn't sure if I was supposed to use git or hg... I went with git since the repo appears to be on Github.

So I created a bunch of JS files and added a do_test method in the classes that didn't have one.
The tests are passing on my machine.

I'm not 100% satisfied with what I did in test_screenshot_path.py (I'm reading the JS test, replacing %(screenshot_name)s by the actual name, and then saving this to a temp file which I then pass to Mozmill.create... seems a little convoluted) so if you have any suggestions I'm all ears.

I also wonder if it would be better to have a common signature for do_test in all the test cases?

Let me know what you think.
(Reporter)

Comment 30

5 years ago
Comment on attachment 8338195 [details] [diff] [review]
Ok first attempt at submitting a patch, not sure if this is what I'm supposed to do here.

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

Corentin, I really like what you did here! A lot of the changes are done very well. As you pointed out there is some code which needs to be improved. So please see my inline comments.

::: mutt/mutt/tests/python/js-modules/newEmptyFunction.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var test_something = function() {}

nit: Now that we have a dedicated file lets format it correctly so that it applies to our coding styles. It means just move the last closing bracket into its own line.

::: mutt/mutt/tests/python/js-modules/testScreenshotPath.js
@@ +7,5 @@
> +}
> +
> +function test() {
> +  var backButton = findElement.ID(controller.window.document, 'back-button');
> +  controller.screenshot(backButton, '%(screenshot_name)s', true);

Yes, this doesn't work. So I would suggest we update the test and transfer the screenshot name via the persisted object to the test.

::: mutt/mutt/tests/python/test_api.py
@@ +30,5 @@
>  
>          self.assertEqual(len(results.passes), 1)
>  
>      def test_create_args_by_reference(self):
> +        relpath = os.path.join("js-modules", "newEmptyFunction.js")

Please use `testpath` here.

::: mutt/mutt/tests/python/test_screenshot_path.py
@@ +21,5 @@
> +        testpath_raw = os.path.join(abspath, relpath)
> +
> +        # read the JS test and replace screenshot_name
> +        with open(testpath_raw) as f:
> +            test = f.read() % dict(screenshot_name=screenshot_name)

As mentioned earlier just use persisted so we don't have to create another temporary file.
Attachment #8338195 - Flags: review?(hskupin) → review-
(Reporter)

Comment 31

5 years ago
(In reply to Corentin Smith (:cosmith) from comment #29)
> Looks like it worked, I wasn't sure if I was supposed to use git or hg... I
> went with git since the repo appears to be on Github.

Yes, and all the right way even as git patch. Good start!

> I also wonder if it would be better to have a common signature for do_test
> in all the test cases?

Yes, absolutely. That's something I already wanted to have for a while. I would suggest we get a new bug filed which probably introduces a new Mutt testcase class which subclasses unittest.TestCase. So by default we can have a common do_test method and for tests which are performing special actions, they can override it. Would you mind to file that enhancement as bug?
(Assignee)

Comment 32

5 years ago
Thanks for the review, I will submit the fixes shortly.

> I would suggest we get a new bug filed which probably introduces a new Mutt 
> testcase class which subclasses unittest.TestCase. So by default we can have
> a common do_test method and for tests which are performing special actions,
> they can override it. Would you mind to file that enhancement as bug?

That's what I was thinking as well, it will reduce code duplication.
(Assignee)

Comment 33

5 years ago
Created attachment 8340003 [details] [diff] [review]
Move JS code into separate test files

I used the persisted object to send the screenshot name, and added the newline in newEmptyFunction.js.

I'm not sure what you meant by this though:

> ::: mutt/mutt/tests/python/test_api.py
> @@ +30,5 @@
> >  
> >          self.assertEqual(len(results.passes), 1)
> >  
> >      def test_create_args_by_reference(self):
> > +        relpath = os.path.join("js-modules", "newEmptyFunction.js")
> 
> 
> Please use `testpath` here.

Do you want me to replace `relpath` by `testpath` everywhere? Or just in this file?
Attachment #8338195 - Attachment is obsolete: true
Attachment #8340003 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Blocks: 944471
(Reporter)

Updated

5 years ago
No longer blocks: 944471
(Assignee)

Comment 34

5 years ago
SO, do you think it's better to close this bug and work directly on Bug 944471 or do you want to merge my patch here before?

(I personally think having something is better than nothing, so I'm in favor of finishing on this one and then moving to the other one; but whatever you guys think is best)
(Reporter)

Comment 35

5 years ago
We should first finish the patch on this bug and get it landed. Once done bug 944471 would be a great follow-up.

(In reply to Corentin Smith (:cosmith) from comment #33)
> > >      def test_create_args_by_reference(self):
> > > +        relpath = os.path.join("js-modules", "newEmptyFunction.js")
> > 
> > 
> > Please use `testpath` here.
> 
> Do you want me to replace `relpath` by `testpath` everywhere? Or just in
> this file?

AFAIK we are using testpath in the other files already, so we should be in sync with all the tests.
(Reporter)

Comment 36

5 years ago
Comment on attachment 8340003 [details] [diff] [review]
Move JS code into separate test files

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

Resetting review request for an update of the patch.
Attachment #8340003 - Flags: review?(hskupin)
(Assignee)

Comment 37

5 years ago
Created attachment 8343071 [details] [diff] [review]
Move JS code into separate test files (reviewed)

I replaced `relpath` by `testpath`.
Attachment #8340003 - Attachment is obsolete: true
Attachment #8343071 - Flags: review?(hskupin)
(Reporter)

Comment 38

5 years ago
Comment on attachment 8343071 [details] [diff] [review]
Move JS code into separate test files (reviewed)

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

Looks way better now. There are still some items left I would like to see addressed before we can land it. Please see my inline comments.

::: mutt/mutt/tests/python/js-modules/newEmptyFunction.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var test_something = function() {

I missed that in my last review, but please do not make use of the 'var foobar = function()' declaration. Instead declare functions directly with 'function foobar()'. This applies to all newly added JS files in this patch.

::: mutt/mutt/tests/python/test_screenshot_path.py
@@ +15,4 @@
>  
>  class ScreenshotPathTest(unittest.TestCase):
>  
> +    def do_test(self, screenshot_name, screenshots_path=""):

Please use the `persisted=None` parameter to pass in persisted values as already done in test_addons.py.

@@ +31,4 @@
>  
>      def test_screenshot_with_custom_path(self):
>          screenshot_name = str(uuid.uuid4())
> +        screenshots_path = tempfile.mkdtemp()

I would move this into a setUp() method.

@@ +37,4 @@
>          screenshots = results.screenshots
>  
> +        self.assertEqual(len(screenshots), 1)
> +        screenshot = os.path.join(screenshots_path, '%s.jpg' % screenshot_name)

I would pass in the correct path from the test method.

@@ +40,5 @@
> +        screenshot = os.path.join(screenshots_path, '%s.jpg' % screenshot_name)
> +        self.assertEqual(screenshots[0]['filename'], screenshot)
> +        self.assertTrue(os.path.isfile(screenshot))
> +
> +        shutil.rmtree(screenshots_path)

The removal I would do in a tearDown method to be sure it gets really deleted in case of a failure.

@@ +51,5 @@
> +        screenshots_path = screenshots[0]['filename'].rpartition(os.path.sep)[0]
> +
> +        self.assertEqual(len(screenshots), 1)
> +        self.assertIn(tempfile.gettempdir(), screenshots[0]['filename'])
> +        self.assertTrue(screenshots[0]['filename'].endswith('%s.jpg' % screenshot_name))

I think that check is obsolete when you check for existence in the next step, just replace its compare value to the constructed one for this case.
Attachment #8343071 - Flags: review?(hskupin) → review-
(Assignee)

Comment 39

5 years ago
Created attachment 8347648 [details] [diff] [review]
Move JS code into separate test files (updated 12-14)

I updated my patch based on your review. Should I keep sending full patches this way (making previous patches obsolete) or submit patches based on the previous one every time?
Attachment #8343071 - Attachment is obsolete: true
Attachment #8347648 - Flags: review?(hskupin)
(Reporter)

Comment 40

5 years ago
(In reply to Corentin Smith (:cosmith) from comment #39)
> I updated my patch based on your review. Should I keep sending full patches
> this way (making previous patches obsolete) or submit patches based on the
> previous one every time?

Please always submit full patches. Older ones will be marked as obsolete as you have already mentioned. I use the interdiff tool to check for differences, e.g.:

https://bugzilla.mozilla.org/attachment.cgi?oldid=8343071&action=interdiff&newid=8347648&headers=1
(Reporter)

Comment 41

5 years ago
Comment on attachment 8347648 [details] [diff] [review]
Move JS code into separate test files (updated 12-14)

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

::: mutt/mutt/tests/python/test_screenshot_path.py
@@ +21,3 @@
>          self.screenshots_path = tempfile.mkdtemp()
> +
> +    def do_test(self, screenshot_name, screenshots_path="", persisted=None):

screenshot_name is not used anymore. So please remove it.

@@ +35,5 @@
>  
> +        screenshot = results.screenshots[0]['filename']
> +
> +        self.assertEqual(len(results.screenshots), 1)
> +        self.assertTrue(os.path.isfile(screenshot))

I would do those asserts in the test function itself but not in do_test(). As we return the results everything will also be available there.

@@ +56,2 @@
>  
> +        self.screenshots_path = screenshot.rpartition(os.path.sep)[0]

It would be better to use os.path.dirname() here.
Attachment #8347648 - Flags: review?(hskupin) → review-
(Assignee)

Comment 42

5 years ago
Created attachment 8349854 [details] [diff] [review]
Updated patch (12-19)

Updated following your review.
Attachment #8347648 - Attachment is obsolete: true
Attachment #8349854 - Flags: review?(hskupin)
(Reporter)

Updated

4 years ago
Attachment #8349854 - Attachment is patch: true
(Reporter)

Comment 43

4 years ago
Comment on attachment 8349854 [details] [diff] [review]
Updated patch (12-19)

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

Ẁe are getting close. Looks like one remaining update is necessary before we can land this patch. Please see my inline comments. Also please check your editor regarding line endings. This patch contains Windows line endings but not Linux ones. The latter we prefer.

::: mutt/mutt/tests/python/test_screenshot_path.py
@@ +35,4 @@
>  
> +        screenshot = results.screenshots[0]['filename']
> +
> +        return (results, screenshot)

You can simply return results only. `screenshot` you can extract when it is necessary in line 44 and 55.

@@ +54,5 @@
> +        self.assertEqual(len(results.screenshots), 1)
> +        self.assertTrue(os.path.isfile(screenshot))
> +
> +        self.screenshots_path = os.path.dirname(screenshot)
> +        self.assertIn(tempfile.gettempdir(), screenshot)

Shouldn't the tempdir compared to `screenshots_path?
Attachment #8349854 - Flags: review?(hskupin) → review-
(Assignee)

Comment 44

4 years ago
> Shouldn't the tempdir compared to `screenshots_path`?

Given that self.screenshots_path is directly derived from screenshot, it shouldn't make any difference wether we compare the tempdir to screenshot or self.screenshot_path (notice I used assertIn, not assertEqual, so the result is correct).
(Assignee)

Comment 45

4 years ago
Created attachment 8358843 [details] [diff] [review]
Updated patch (01-11)

Removed `screenshot` from the return value of do_test
Attachment #8349854 - Attachment is obsolete: true
Attachment #8358843 - Flags: review?(hskupin)
(Assignee)

Comment 46

4 years ago
Created attachment 8358845 [details] [diff] [review]
Fixed patch (01-11)

I messed up the previous patch, sorry about that.
Attachment #8358843 - Attachment is obsolete: true
Attachment #8358843 - Flags: review?(hskupin)
Attachment #8358845 - Flags: review?(hskupin)
(Assignee)

Comment 47

4 years ago
Apparently something's wrong my the patch, diffing against a previous patch gives 

> Warning: interdiff encountered errors while comparing these patches.
> No valid patch files were found in the attachment.

Not sure what's wrong though...
(Reporter)

Comment 48

4 years ago
Comment on attachment 8358845 [details] [diff] [review]
Fixed patch (01-11)

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

We are close. Only some minor issues left here. Please see my inline comments.

::: mutt/mutt/tests/python/test_logger_listener.py
@@ +33,5 @@
> +        self.assertIn("DEBUG", debug_data.getvalue())
> +        self.assertIn("TEST-START", debug_data.getvalue())
> +        self.assertIn("TEST-PASS", debug_data.getvalue())
> +        self.assertNotIn("DEBUG", info_data.getvalue())
> +    def test_logger_listener(self):

nit: please add a blank line before the method definition. This also applies to any other instances in this file and the other modified tests.

For that please see http://www.python.org/dev/peps/pep-0008/#blank-lines

::: mutt/mutt/tests/python/test_screenshot_path.py
@@ +48,5 @@
> +        screenshot = results.screenshots[0]['filename']
> +        self.assertEqual(len(results.screenshots), 1)
> +        self.assertTrue(os.path.isfile(screenshot))
> +
> +        self.screenshots_path = os.path.dirname(screenshot)

I haven't seen this the last time but here you are overwriting the formerly path returned by the mkdtmp() call in setUp(). That means in tearDown you don't remove this temporary folder. To get around that you shouldn't create the folder or assign the path in setUp but in the test methods.
Attachment #8358845 - Flags: review?(hskupin) → review-
(Assignee)

Comment 49

4 years ago
Created attachment 8369120 [details] [diff] [review]
0001-Move-JS-code-into-separate-files-to-get-rid-of-make_.patch

Sorry for the delay. I think we should be good now, although I think I already had white lines between functions.
Attachment #8358845 - Attachment is obsolete: true
Attachment #8369120 - Flags: review?(hskupin)
(Reporter)

Comment 50

4 years ago
Comment on attachment 8369120 [details] [diff] [review]
0001-Move-JS-code-into-separate-files-to-get-rid-of-make_.patch

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

Looks way better. There is a single thing left which you will have to fix. Once this is done we can get the patch landed!

::: mutt/mutt/tests/python/test_screenshot_path.py
@@ +46,5 @@
> +                                         '%s.jpg' % self.screenshot_name)
> +        self.assertEqual(wanted_screenshot, screenshot)
> +        self.assertTrue(os.path.isfile(screenshot))
> +
> +        shutil.rmtree(screenshots_path)

So with this rewrite you would still leave the created temporary folder around if you hit an assertion e.g. in line 42. The removal of the path really has to happen in tearDown. Everything else is not reliable. Therefor you should continue in declaring self.screenshots_path in setUp but as None. Then the test can set the appropriate path as needed.
Attachment #8369120 - Flags: review?(hskupin) → review-
(Assignee)

Comment 51

4 years ago
Yes of course... stupid mistake. I'll try to resubmit quickly.
(Assignee)

Comment 52

4 years ago
Created attachment 8380779 [details] [diff] [review]
patch-02-24.patch

Sorry for the delay... I hope everything is right this time.
Attachment #8369120 - Attachment is obsolete: true
Attachment #8380779 - Flags: review?(hskupin)
(Reporter)

Updated

4 years ago
Attachment #720963 - Attachment is obsolete: true
(Reporter)

Comment 53

4 years ago
Comment on attachment 8380779 [details] [diff] [review]
patch-02-24.patch

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

Sorry for the delay of the review, but was kinda swamped with review requests. All looks fine here now, except two minor things. Would be great if you can get those updated. Also make sure to pull again from master to check if the patch still applies. A lot of code has been changed in the past weeks.

r=me with the nits addressed.

::: mutt/mutt/tests/python/test_screenshot_path.py
@@ +17,5 @@
>  
> +    def setUp(self):
> +        self.screenshot_name = str(uuid.uuid4())
> +        self.persisted = {"screenshotName": self.screenshot_name}
> +        self.screenshots_path = None

Mind moving this up right below screenshot_name? That way we have both related settings located next to each other. Also I would add a blank line before the persisted line.

@@ +60,3 @@
>  
>      def tearDown(self):
> +        shutil.rmtree(self.screenshots_path)

Mind changing this quickly to mozfile.remove()? It's more safe in removing files.
Attachment #8380779 - Flags: review?(hskupin) → review+
(Assignee)

Comment 54

4 years ago
Created attachment 8392397 [details] [diff] [review]
Final patch (hopefully)

Ok I hope this can be merged! All tests are passing on the latest master.
Attachment #8380779 - Attachment is obsolete: true
Attachment #8392397 - Flags: review?(hskupin)
(Reporter)

Comment 55

4 years ago
Comment on attachment 8392397 [details] [diff] [review]
Final patch (hopefully)

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

::: mutt/mutt/tests/python/test_screenshot_path.py
@@ +9,4 @@
>  import tempfile
>  import uuid
>  
> +import mozfile

Given the addition of this external dependency you will also have to add the package to the dependencies in the setup.py of the Mutt package. With that fixed you get my r+!
Attachment #8392397 - Flags: review?(hskupin) → review-
(Assignee)

Comment 56

4 years ago
Created attachment 8395199 [details] [diff] [review]
Updated deps

I added 'mozfile' in the dependencies list, I'm not sure if there is anything else to do.
Attachment #8392397 - Attachment is obsolete: true
Attachment #8395199 - Flags: review?(hskupin)
(Reporter)

Comment 57

4 years ago
Comment on attachment 8395199 [details] [diff] [review]
Updated deps

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

Usually we list dependencies alphabetically in the setup.py file but given that we do not distribute mutt via pypi, I'm fine with it.
Attachment #8395199 - Flags: review?(hskupin) → review+
(Reporter)

Comment 58

4 years ago
Comment on attachment 8395199 [details] [diff] [review]
Updated deps

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

Actually applying this patch gives me a white-space error via git. Looks like you made an incorrect change in report.py. Please remove that file from the patch. It's not necessary. While doing that you will also update the setup.py file as mentioned above.

With those changes you will still get my r+. Can you port it forward once the new patch is ready.

Thanks Corentin for all the work on this bug! I hope it was helpful to give you some more insights into the Mozmill test framework. I would love if you have interests for something else, I could mentor you. Please let me know. Also I would like to mention that we have 2 automation training days this week. You might want to stop by on IRC in #automation today and on Wednesday.
(Assignee)

Comment 59

4 years ago
Created attachment 8397310 [details] [diff] [review]
0001-Final-patch.patch

Sorry for the previous patch, I don't know what happened.

I applied this one before sending and it looks fine. I also changed the dependencies list to keep them in alphabetical order.

Thanks for reviewing so many times! I'd be glad to start working on something else if you have any suggestions. (Maybe Bug 944471 ?)
Attachment #8395199 - Attachment is obsolete: true
Attachment #8397310 - Flags: review?(hskupin)
(Reporter)

Comment 60

4 years ago
(In reply to Corentin Smith (:cosmith) from comment #59)
> Thanks for reviewing so many times! I'd be glad to start working on
> something else if you have any suggestions. (Maybe Bug 944471 ?)

Absolutely no problems! I'm happy if my feedback is helpful and you can increase your experience. Whatever I can do to do more let me know. Also it's great that you want to continue! So yes, the proposed bug seems to be perfect! Thanks!
(Reporter)

Comment 61

4 years ago
Comment on attachment 8397310 [details] [diff] [review]
0001-Final-patch.patch

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

All looks fine. Just one more thing for the future. When you rebase or squash your commits, please make sure to have the bug summary in your commit message. Usually it's best to set it the first time you commit on your branch. It will stick later even when you rebase. I updated the commit message for the landed patch. Please have a look at it to see the format of the message.
Attachment #8397310 - Flags: review?(hskupin) → review+
(Reporter)

Comment 62

4 years ago
Landed on master for mozmill-2.1 release:
https://github.com/mozilla/mozmill/commit/74474043cc9ac709b67d14fd2f17dabde0b03e83
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [mentor=whimboo][lang=py][good first bug] → [mentor=whimboo][lang=py][good first bug][mozmill-2.1]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.