853005 - Update automation script should exit with a non-zero exit code if unable to get update channel

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect, P2)

Description

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

Today we have seen the fallback tests pass but the direct update tests fail when attempting to get the update channel. The exception was caught and printed to console, but this meant that the Jenkins job passed. We should rethrow this exception at the end of the testrun.

Comment 1

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

We should get this fixed soonish. Sounds like we miss to re-raise the exception.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Just to make sure, should we raise all exceptions or just for update testrun?

Comment 3

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

If any exceptions occur that relate to tests failing or being unable to run we should catch them in order to allow any subsequent processes to finish (running more tests, submitting reports, etc) and then re-raise the exceptions at the end. This will cause the process to exit in a negative state and cause a CI build to fail.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

I have tested each testrun, make it fail and I got the exceptions in the console.
For functional I changed the path for normal tests to be wrong, so at the beginning I got the error, then restart tests started and in the end after their results, the exception again.

Comment 5

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

Comment on attachment 731144 [details] [diff] [review]
patch v1

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

That looks fine as far as I can see. Some nits see my inline comments. I would like to see a final review from Dave with the next iteration. Something we should also consider is to rework all the exception handling, but that can happen post Mozmill 2.0 with our new automation scripts.

::: libs/testrun.py
@@ +583,5 @@
>  
> +        # If an exception has been thrown for any of the builds under test
> +        # re-throw the exact same exception again. We just need it for the
> +        # exit code
> +        if self.last_exception:

The comment here is wrong. This code doesn't know anything about which builds are getting executed. This also applies to all the other comments.

@@ +965,5 @@
>              self.test_path = os.path.join('tests', 'update', folder)
>              TestRun.run_tests(self)
>          except Exception, e:
>              print "Execution of test-run aborted: %s" % str(e)
> +            #self.last_exception = e

Any reason why this has been commented out?

Comment 6

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated the comments and uncommented that line, I forgot it while testing.

Comment 7

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

Comment on attachment 732708 [details] [diff] [review]
patch v2

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

As mentioned, it should be enough to just set self.last_exception as the `run` method should detect this has been set and raise it again.

::: libs/testrun.py
@@ +582,5 @@
>                      print "*** Failed to remove target add-on '%s'." % self.target_addon
>  
> +        # If an exception has been thrown, re-throw it again.
> +        # We just need it for the exit code
> +        if self.last_exception:

We shouldn't need to re-throw the exception here. As we've set self.last_exception we will already re-raise it in `TestRun.run`.

@@ +675,5 @@
> +            self.last_exception = e
> +
> +        # If an exception has been thrown, re-throw it again.
> +        # We just need it for the exit code
> +        if self.last_exception:

As above, setting self.last_exception should be enough.

@@ +745,5 @@
>          except Exception, e:
>              print str(e)
> +            self.last_exception = e
> +
> +        # If an exception has been thrown, re-throw it again.

Again, there should be no need to raise here.

@@ +773,2 @@
>  
> +        # If an exception has been thrown, re-throw it again.

And here

@@ +806,2 @@
>  
> +        # If an exception has been thrown, re-throw it again.

And here.

@@ +947,5 @@
>              else:
>                  self.results.append(self.build_wiki_entry(direct_data))
>                  self.results.append(self.build_wiki_entry(fallback_data))
>  
> +        # If an exception has been thrown, re-throw it again.

And here.

@@ +975,5 @@
>              return data
>  
> +        # If an exception has been thrown, re-throw it again.
> +        # We just need it for the exit code
> +        if self.last_exception:

This code will never be reached due to the finally and return statement. It's not needed here anyway.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2.1

Updated to only save the exception, not re-throw it.

Comment 9

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

Comment on attachment 733300 [details] [diff] [review]
patch v2.1

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

http://hg.mozilla.org/qa/mozmill-automation/rev/952c967f4779

Andreea, can you please also update the code in our mozmill-automation repository for Mozmill 2.0 please? You can find it here: https://github.com/whimboo/mozmill-automation

Comment 10

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

This is not fully fixed. While checking for the jsbridge error I have seen the following:

Execution of test-run aborted: Sorry, cannot connect to jsbridge extension, port 24242
'updates'
*** Uninstalling c:\users\mozauto\appdata\local\temp\tmpafd0pa.binary\
*** Removing repository 'c:\users\mozauto\appdata\local\temp\tmpwd0glv.mozmill-tests'
Traceback (most recent call last):
  File "mozmill-automation/testrun_update.py", line 51, in <module>
    main()
  File "mozmill-automation/testrun_update.py", line 46, in main
    UpdateTestRun().run()
  File "c:\testing\ondemand_update\mozmill-automation\libs\testrun.py", line 941, in run
    TestRun.run(self)
  File "c:\testing\ondemand_update\mozmill-automation\libs\testrun.py", line 439, in run
    raise self.last_exception
KeyError: 'updates'

I will backout the patch given that I'm not sure if it harms us in any other way.

Backout:
http://hg.mozilla.org/qa/mozmill-automation/rev/165be248a5b5

Comment 11

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

This looks genuine, in that 'updates' didn't appear in the report and therefore an exception was thrown. I wouldn't have backed this out as it was already providing us value.

Comment 12

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

Oh, my. You are right. My bad. We should file follow-up bugs to get this fixed. Correct.

Relanded as:
http://hg.mozilla.org/qa/mozmill-automation/rev/4f181c64c91c

Comment 13

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

Andreea, what's left here?

Comment 14

[Andreea Matei [:AndreeaMatei]]

The same changes for 2.0. I've updated the pull request:
https://github.com/mozilla/mozmill-automation/pull/32

Comment 15

[Andreea Matei [:AndreeaMatei]]

This have been fixed in 2.0 as well.