868435 - testrun_addons.py without `--with-untrusted` arg appears to still attempt run

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

[Jonathan French (:jfrench)]

Begat from comment in bug 848649
https://bugzilla.mozilla.org/show_bug.cgi?id=848649#c71

mozmill-automation's testrun_addons.py (at least on Windows) appears to fail unless the `--with-untrusted` argument is used. A good warning message below appears to be issued during the run process, which is fine. So maybe this isn't a bug after all. 

Feel free to resolve the bug as invalid, if that is the case.

But entering just in case there is a more elegant way of preventing the run, since instead of stopping at the beginning, it clones the repository before, and then appears to attempt the test run. Having already issued a warning about a run that it appears to know isn't going to work.

To reproduce:
$ ./testrun_addons.py "C:\Program Files\Mozilla Firefox Default\firefox.exe" --repository /c/tmp/blargon/mozmill-tests/ --report=http://mozmill-crowd.blargon7.com/db/


*** Download URL for 'selenium-ide-editor-1.10.0.xpi' is not trusted.
*** Use --with-untrusted to force testing this add-on.
'AddonsTestRun' object has no attribute 'target_addon'
*** Removing repository 'c:\docume~1\jfrench\locals~1\temp\tmpg1ozmp.mozmill-tests'
Traceback (most recent call last):
  File "./testrun_addons.py", line 51, in <module>
    main()
  File "./testrun_addons.py", line 46, in main
    AddonsTestRun().run()
  File "c:\tmp\mozmill-automation\libs\testrun.py", line 439, in run
    raise self.last_exception
AttributeError: 'AddonsTestRun' object has no attribute 'target_addon'

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

This is a regression from bug 853005 given that we re-throw exceptions now. We most likely shouldn't do this here, or most likely add a better failure message like: No add-ons to test have been found.

This is a good mentored bug.

Comment 2

[MikeLing]

Hi I am interested in working on this bug,but it's my first time to work on with debug,can anybody guide me on how to get started with it?Thanks a lot.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Just clone the repository as given in the URL field and execute the given testrun script. You should see the failure Jonathan has been reported her.

I wouldn't assign the bug to you yet, given that you did the same comment on a couple of other bugs.

Comment 4

[Jonathan French (:jfrench)]

A minor note, with a mozmill-env updated from 1.5.21 to 2.0rc3 latest, the same command won't clone the repository unnecessarily before failing; instead it will fail right off the bat, but with a slightly different error. This may be unrelated and outside the scope of this bugfix, but including it here in case of interest.

Traceback (most recent call last):
  File "./testrun_addons.py", line 41, in <module>
    from libs.testrun import *
  File "c:\tmp\mozmill-automation\libs\testrun.py", line 67, in <module>
    'thunderbird' : mozmill.ThunderbirdCLI,
AttributeError: 'module' object has no attribute 'ThunderbirdCLI'

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Jonathan, if you see any failure with the mozmill 2.0 automation scripts please get those filed as issue in my github repository: https://github.com/whimboo/mozmill-automation

Comment 6

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #4)
> Traceback (most recent call last):
>   File "./testrun_addons.py", line 41, in <module>
>     from libs.testrun import *
>   File "c:\tmp\mozmill-automation\libs\testrun.py", line 67, in <module>
>     'thunderbird' : mozmill.ThunderbirdCLI,
> AttributeError: 'module' object has no attribute 'ThunderbirdCLI'

I checked and confirmed this was pilot error on my part. I was attempting to run testrun_addons.py which is in a 1.5 distribution, within a mozmill-env shell which had been updated to 2.0. Dave indicated on IRC they are not backward/forward compatible.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Ok, thank you for the feedback. Closing the bug appropriately.

Comment 8

[Jonathan French (:jfrench)]

Actually Comment 6 was just about a side-investigation trying to compare 2.0 behavior with 1.5. 

With 1.5(the original description at the top), the behavior observed still exists. mozmill-automation goes through the process of cloning the repo uncecessarily before determining the required arg --with-untrusted is missing.

Ideally, the script should fail right at the beginning, echo the correct usage, and not clone the repo.

Comment 9

[Jonathan French (:jfrench)]

And I forgot to add; the traceback attribute error as listed in the Description still occurs also, with testrun_addons.py, when --with-untrusted is missing.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

No, we always have to clone the repository with the tests first. We don't know before-hand which of the add-ons we are testing are located on OMO and which not. So there is no way to get around that. Does that clarifies the confusion?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Jonathan French (:jfrench) from comment #9)
> And I forgot to add; the traceback attribute error as listed in the
> Description still occurs also, with testrun_addons.py, when --with-untrusted
> is missing.

With the 1.5 or 2.0 branch? Or in both?

Comment 12

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> No, we always have to clone the repository with the tests first. We don't
> know before-hand which of the add-ons we are testing are located on OMO and
> which not. So there is no way to get around that. Does that clarifies the
> confusion?

Yup got it, thanks Henrik.

Comment 13

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> (In reply to Jonathan French (:jfrench) from comment #9)
> > And I forgot to add; the traceback attribute error as listed in the
> > Description still occurs also, with testrun_addons.py, when --with-untrusted
> > is missing.
> 
> With the 1.5 or 2.0 branch? Or in both?

With 1.5, at least. I am not able to configure mozmill-automation via a 2.0 run.cmd shell (ie. mozmill-env updated to 2.0) and installing the requirements.txt in mozmill-automation successfully. For a variety of reasons which I won't get into here.

But the error in the Description occurs in 1.5 while running testrun_addons.py in a mozmill-env 1.5.21 run.cmd shell; so the bug could be considered as a 1.5 issue if that had merit. If not, no problems.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Ok, so if that is really happening in the 1.5 version we might want to get this fixed. It's not a high priority at the moment. But if someone is interested in I'm happy to review the patch. It looks like that we missed to declare target_addon in the right scope.

Mike, do you still have interests to fix this bug?

Comment 15

[Jarek Śmiejczak [:jotes]]

Hi, I can start working on this ticket if that's ok.

Comment 16

[Jonathan French (:jfrench)]

Sure Jarek. I can get you started.

First download and unpack the 1.5.21 mozmill-env for your particular platform
http://mozqa.com/mozmill-env

Then using mercurial, clone this repo below which contains the testrun python scripts
http://hg.mozilla.org/qa/mozmill-automation

Then download a current installer of Firefox Nightly somewhere but don't run it
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
eg. /2013-08-12-03-02-09-mozilla-central/firefox-26.0a1...

I also cloned the mozmill-tests repo locally, and that was my --repository=
http://hg.mozilla.org/qa/mozmill-tests

Then run in the mozmill-env environment for your platform; navigate in that shell to the mercurial testrun repo and try to run in the form in the Description (ie. without --with-untrusted).

eg. ./testrun_addons.py --repository=(repo) (your local installer)

Then see if you observe the additional error messages in the Description.

'AddonsTestRun' object has no attribute 'target_addon'
Traceback (most recent call last):
  File "./testrun_addons.py", line 51, in <module>
    main()
  File "./testrun_addons.py", line 46, in main
    AddonsTestRun().run()
  File "c:\tmp\mozmill-automation\libs\testrun.py", line 439, in run
    raise self.last_exception
AttributeError: 'AddonsTestRun' object has no attribute 'target_addon'

I just tested it myself, and reproduced it on Windows XP SP3.

I defer to Henrik if I've misstated anything in the setup.

The goal of this bug I think would be to see if we could get rid of that 9 line error. So that the final four lines of the run attempt would be:

*** Download URL for 'selenium-ide-editor-1.10.0.xpi' is not trusted.
*** Use --with-untrusted to force testing this add-on.
*** Uninstalling ...binary\
*** Removing repository '...repo

This would also help users when running, since they will see the error and the missing argument more clearly.

Comment 17

[Jarek Śmiejczak [:jotes]]

Attachment: bug_868435.diff

Thank you Jonathan for so verbose introduction! It helped me a lot.
Sorry for so late response, but i was a little busy last days.
I'm adding my patch which solves this problem, however in current state of code i'm not sure if it's enough for Henrik.

Comment 18

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

Comment on attachment 798629 [details] [diff] [review]
bug_868435.diff

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

Don't forget to request feedback or review when attaching patches. Looking at this, we're not adding the information regarding the failure to the user. I suspect we want to catch and log the failure, and if it's due to the addon being untrusted, include the following print statements:

*** Download URL for %ADDON% is not trusted.
*** Use --with-untrusted to force testing this add-on.

Comment 19

[Jarek Śmiejczak [:jotes]]

:davehunt
Sorry about this missing request, i wasn't sure how to do this in bugzilla interface.

Code was a little bit more complicated and it took me a while to understand more what's happening here. After thought, i decided to break defensive style of this code and move some sections outside try: finally:.

Comment 20

[Jarek Śmiejczak [:jotes]]

Attachment: bug_868435.diff

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 798951 [details] [diff] [review]
bug_868435.diff

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

Jonathan is not a reviewer, so you can't ask him to do a review. Lets have another round of feedback. So I will add myself.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 798951 [details] [diff] [review]
bug_868435.diff

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

So I have checked that patch and what I can see, the move of the code will not help us here. Those lines are not the cause of this exception. So what I would suggest is that you figure out where this exception gets thrown first. Means where self.target_addon gets accessed when it is not set. That will help us to find the proper fix for this issue.

Comment 23

[Jarek Śmiejczak [:jotes]]

Attachment: bug_868435.diff

So after talk with Henrik on irc we decided to revert previous patch, because it could be a too large interference into current codebase.
A flow of code inside this loop is hard to explain in small diff because some mechanisms of code-flow are overshadowed by nested exception handling in loop.
We must reset target_addon property in every iteration of loop to avoid strange exception handling in finally.

I think that previous state of code was a little buggy. Because if target_addons would have following items: [<trusted addon>, <untrusted addon>] then finally block would try to remove trusted addon twice.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Jarek Śmiejczak from comment #23)
> I think that previous state of code was a little buggy. Because if
> target_addons would have following items: [<trusted addon>, <untrusted
> addon>] then finally block would try to remove trusted addon twice.

That's a perfect investigation and the outcome I have expected! Having another look at the code is not that bad. Well done Jarek! I will have a look at the patch now.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 800461 [details] [diff] [review]
bug_868435.diff

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

See my inline comment for the remaining thing to do. We would also need the same fix for our master and hotfix-2.0 branch of the new automation repository (https://github.com/mozilla/mozmill-automation/). Please file an issue for that. Thanks.

::: libs/testrun.py
@@ +529,4 @@
>                  # Get the download URL
>                  self._addon_path = os.path.join('tests', 'addons', self._addon)
>                  url = self.get_download_url()
> +                self.target_addon = ''

Personally I would move this line up so it will be the first one inside the try statement. It's a bit hidden at this location. Also I would set it to ´None´, which is better to check.

nit: A blank line around it would be nice to have, so it is separated from the following code.

Comment 26

[Jarek Śmiejczak [:jotes]]

Attachment: bug_868435.diff

I've updated patch according to whimboo's notes and i've added corresponding issue on github with PR for 2.0 and master.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 801280 [details] [diff] [review]
bug_868435.diff

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

Looks way better as the last versions! But please update the commit message according to:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests#Commit_message

With that changed it will get my r+. Thanks.

Comment 28

[Jarek Śmiejczak [:jotes]]

Attachment: bug_868435.full.diff

Sorry, I'm relatively new in whole hg patch process so i did patch via `hg diff`.
It's my first attempt via hg's message queue.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Comment on attachment 801505 [details] [diff] [review]
bug_868435.full.diff

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

That looks better, indeed! ;) Thanks for the patch. Please make sure to add patches also for the hotfix-1.5 and hotfix-2.0 branches on the github mozilla-automation repository.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Landed as: http://hg.mozilla.org/qa/mozmill-automation/rev/c6bd488ee1d9