897382 - Include the update channel in the report even for failed update tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

At the moment the update channel is only set in the update test report if an update was available. The channel is not dependent on there being an update, so we should include this regardless.

  if (this.activeUpdate) {
    info = {
      buildid : this.activeUpdate.buildID,
      channel : this.channel, <-- this can be moved outside of the if statement
      is_complete : this.isCompleteUpdate,
      size : this.activeUpdate.selectedPatch.size,
      type : this.activeUpdate.type,
      url_mirror : this.activeUpdate.selectedPatch.finalURL || "n/a",
      download_duration : this._downloadDuration,
      version : this.activeUpdate.version
    };
  }

http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/software-update.js#l211

Comment 1

[Sushant Hiray [:sushant]]

Attachment: Bug-897382: Removed channel from the conditional statement

I've removed channel from the conditional statement. Is there more to be done apart from shifting the channel?

Comment 2

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

Comment on attachment 780786 [details] [diff] [review]
Bug-897382: Removed channel from the conditional statement

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

Thanks Sushant, I think that is all that's needed. I've added a couple of comments and it would be great to test this. You can do so by running an update test with an invalid or inactive channel and pushing the results to our mozmill-crowd dashboard. If the channel is present in the report this it's fixed.

::: lib/software-update.js
@@ +206,5 @@
>     * Returns information of the active update in the queue.
>     */
>    get patchInfo() {
>      var info = { }
> +	

Please check your patches for extraneous whitespaces.

@@ +218,5 @@
>          download_duration : this._downloadDuration,
>          version : this.activeUpdate.version
>        };
>      }
> +    info.channel=this.channel;

I would move this before the if statement rather than after it. Also, please include a space either side of the equals symbol.

Comment 3

[Sushant Hiray [:sushant]]

I've made the changes as required in the file.
But I'm having problems installing mozmill for the tests. 
I access internet under a proxy server, 
I tried using pip --proxy username:password@servver:port install mozmill but this isn't working for me. Also the firefox addon mentioned on the website seems to be pulled down.
Is there any alternate way to install mozmill?

Comment 4

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

See https://github.com/mozilla/mozmill#installation

Comment 5

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

Or better use our already pre-configured mozmill environment, which you can download here:
https://mozqa.com/mozmill-env/

You only have to unzip it and execute the run script. Then mozmill and all necessary tools will immediately be available.

Comment 6

[Sushant Hiray [:sushant]]

Dave Hunt,
I tried running the testrun_update.py, but I was getting error. 

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

I found a related error here: https://bugzilla.mozilla.org/show_bug.cgi?id=795579#c12
On shifting to cloned automation repo, I got the following error:

./testrun.py: line 29: MOZMILL_TESTS_REPOSITORIES: command not found
./testrun.py: line 32: syntax error near unexpected token `}'
./testrun.py: line 32: `}'

My mozmill version is: 2.0rc4
My mozrunner version id: 5.18

Could you help me in figuring this out! :)

Comment 7

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

Ah, you will need to use Mozmill 1.5.21. You can download an environment for your platform from here: https://mozqa.com/mozmill-env/

Comment 8

[Sushant Hiray [:sushant]]

Davehunt,
I downloaded the environment.
When I ran this command in the environment: ./testrun_update.py  --channel=invalid_channel --report=http://mozmill-crowd.blargon7.com/db/ --application=firefox

It says *** No builds have been specified. Use --help to see all options.

I tried looking into libs/testrun.py where it was specifying the error.
So in _set_binaries(self,value) the value being passed was null, 
hence self.binary was essentially null.

Comment 9

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

(In reply to Sushant Hiray [:sushant] from comment #8)
> Davehunt,
> I downloaded the environment.
> When I ran this command in the environment: ./testrun_update.py 
> --channel=invalid_channel --report=http://mozmill-crowd.blargon7.com/db/
> --application=firefox
> 
> It says *** No builds have been specified. Use --help to see all options.
> 
> I tried looking into libs/testrun.py where it was specifying the error.
> So in _set_binaries(self,value) the value being passed was null, 
> hence self.binary was essentially null.

You need to specify a path to the Firefox build you want to run the tests against. This is the final argument, and is not optional. For example, the command line for me would be:

./testrun_update.py --channel=invalid_channel --report=http://mozmill-crowd.blargon7.com/db/ /Applications/FirefoxNightly.app

You can also omit the --application as firefox is the default.

Comment 10

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

Sushant, does it work for you now?

Comment 11

[Sushant Hiray [:sushant]]

Oops! I thought I had posted the weird problem I'm facing.
When I run ./testrun_update.py -channel=invalid_channel --report=http://mozmill-crowd.blargon7.com/db/ /usr/lib/firefox/firefox    

in mozmill env: I get the following error: http://pastebin.mozilla.org/?diff=2854558

which is quite amusing because when I run python in mozmill env and import mozmill, it works just fine.

Do I need to specify some additional paths to the mozmill lib while running the test cases?

Comment 12

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

We discussed that on IRC and sorted out the problem.

Comment 13

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

Hi Sushant, it's been a while without hearing from you. Are you still interested in working on this bug?

Comment 14

[Sushant Hiray [:sushant]]

Hi Dave,

I've been busy with university work and won't be able to give it time in near future. If someone is interested to work on this feel free to assign, or else I'll work on it once I'm done with my semester.

Comment 15

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

Thanks Sushant. I'll clear the assignee field for now. Let us know if you find time to work on this.

Comment 16

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

As pointed out to me on IRC the code location has been changed. So I will post an updated URL here:
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/software-update.js#l318

Comment 17

[Virginia Thayer (:thayerve)]

I'm interested in working on this bug. 
I see that Sushant submitted a patch but is no longer working on it. My current understanding is that I need to make a patch moving the this.channel line up above the if statement, and test the patch as specified in Comment 2. 
I spoke with Henrik/whimboo in IRC today and found the code at 
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/software-update.js#l318, and I've run Mozmill tests and pushed reports to the mozmill-crowd dashboard before, so I'm pretty sure I can get this done if you'd like to assign it to me. Thanks!

Comment 18

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

Totally! All what you describe above sounds perfect. Good wrap up of the work, which has to be done here. So in case for testing just make sure to test with a latest nightly build, where no updates are available. Currently we do not show the channel name in the dashboard.

Comment 19

[Virginia Thayer (:thayerve)]

Attachment: Bug-897382: Include update channel in report even for failed update tests

Posting first draft of patch. I have tested the code but can't quite tell if it has fixed the problem, or if possibly the bug actually lies with the crowd results dashboard?
I updated Nightly today and ran the test via this script: 

$ testrun_update --repository=~/Sites/bug897382/mozmill-tests/ --report=http://mozmill-crowd.blargon7.com/db/ /Applications/FirefoxNightly.app/

As I understand it, a successful fix would show 'Nightly' (or would it be 'Invalid_Channel'?) in the blank spot next to 'Channel:' here: http://mozmill-crowd.blargon7.com/#/update/report/53ec3148fca67aea1b1c3528f941d286. 

The raw output here does include "channel":"invalid_channel"
http://mozmill-crowd.blargon7.com/db/53ec3148fca67aea1b1c3528f941d286.

So I'm not sure why it's not populating in the dashboard. Any ideas?

Comment 20

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

Comment on attachment 8499724 [details] [diff] [review]
Bug-897382: Include update channel in report even for failed update tests

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

Please see my inline comment why it currently fails for you. With this fixed all should be fine. Please let me know if you have other problems.

::: firefox/lib/software-update.js
@@ +317,4 @@
>     */
>    get patchInfo() {
>      var info = { }
> +    info.channel = this.channel;

So two things regarding this change here. I made a change recently, which is in conflict with your change. `this.channel` is no longer valid, so you want to use `this.updateChannel.channel` here.

Further you can put the assignment directly into the info declaration as property. With that you will have to make sure that the following code (in the if condition) will not overwrite the whole info block again. You will see that when an update exists. So you can update as proposed at the beginning of this paragraph, or we do it after the if condition. I would prefer to get it set directly, and any other code should do `info.xyz = foobar`.

Comment 21

[Virginia Thayer (:thayerve)]

Attachment: Bug-897382: Include update channel in report even for failed update tests

I've remade the patch based on mentor feedback, and the json output does seem to include the channel, as shown in this output from an update test on the latest version of Aurora: http://mozmill-crowd.blargon7.com/db/e5ac82d610ad4751cf2efea915188d2d.
However, the channel is still not showing up in the dashboard here: http://mozmill-crowd.blargon7.com/#/update/report/e5ac82d610ad4751cf2efea915188d2d, and I'm not sure why. Anyhow, I think this patch fixes the problem as the output of update channel no longer depends on whether an update is available. Is there anything else I need to do? 

And can you determine why the channel still isn't populating on the dashboard? Is that something else we should file a bug for?

Comment 22

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

Comment on attachment 8502847 [details] [diff] [review]
Bug-897382: Include update channel in report even for failed update tests

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

(In reply to Virginia Thayer from comment #21)
> I've remade the patch based on mentor feedback, and the json output does
> seem to include the channel, as shown in this output from an update test on
> the latest version of Aurora:
> http://mozmill-crowd.blargon7.com/db/e5ac82d610ad4751cf2efea915188d2d.

Yes, that looks perfect! Thanks a lot. Just check the remaining items to get fixed, then we can push your patch.

> However, the channel is still not showing up in the dashboard here:
> http://mozmill-crowd.blargon7.com/#/update/report/
> e5ac82d610ad4751cf2efea915188d2d, and I'm not sure why. Anyhow, I think this
> patch fixes the problem as the output of update channel no longer depends on
> whether an update is available. Is there anything else I need to do? 

The problem lays actually here:
http://hg.mozilla.org/qa/mozmill-tests/file/b1d199f491d0/firefox/tests/update/testDirectUpdate/testUpdate.js#l74

> And can you determine why the channel still isn't populating on the
> dashboard? Is that something else we should file a bug for?

I checked that and if no update is found, the waitFor() call will raise an exception, and the test method returns. Given that we always want to have the patch info we could add this to the teardownTest function, or better we wrap the waitFor() and download() call into a try/finally block. So that in finally we update the patch info. Please make sure to also update it for the testFallbackUpdate case.

::: firefox/lib/software-update.js
@@ +319,5 @@
> +    var info = {
> +
> +    channel : this.updateChannel.channel;
> +
> +    };

nit: please remove the empty lines around the channel line, and obey the 2 characters of indentation for our code.

@@ +329,5 @@
> +      info.type = this.activeUpdate.type;
> +      info.url_mirror = this.activeUpdate.selectedPatch.finalURL || "n/a";
> +      info.download_duration = this._downloadDuration;
> +      info.version = this.activeUpdate.version;
> +      }

You missed to remove this extra closing bracket. It should have caused a syntax failure for you.

Comment 23

[Virginia Thayer (:thayerve)]

Attachment: Bug-897382: Include update channel in report even for failed update tests

I believe this patch closes this bug. 

I would like to file a new bug to address the other issue, and title it "Update testDirectUpdate and testFallbackUpdate so waitFor() still returns patch info." And that bug could be assigned to me as well. Sound good?

Comment 24

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

Comment on attachment 8503341 [details] [diff] [review]
Bug-897382: Include update channel in report even for failed update tests

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

> I would like to file a new bug to address the other issue, and title it
> "Update testDirectUpdate and testFallbackUpdate so waitFor() still returns
> patch info." And that bug could be assigned to me as well. Sound good?

As you can read in the bug summary, to goal for this bug is to include the channel even for failed update tests. The patch as it is, doesn't completely fix it. So you should really add the remaining code as I have given in the second last comment to your patch. 

For us this would even be better because we have to backport this patch to all the other supported branches. And a combined patch gives lesser headaches if something goes wrong.

For now I remove the review request and will wait for an updated patch. Thanks!

Comment 25

[Virginia Thayer (:thayerve)]

Attachment: updatePatchInfo.patch

Wrap the waitFor() and download() calls into a try/finally block in testDirectUpdate and testFallbackUpdate as recommended in Comment 22. 

However, I am still unable to verify the fix in a test. Not sure what else to try.

Comment 26

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

What do you mean that it doesn't work for you? I just tried the patch and the channel is now a property under patch, even if no update is available because you already use the latest nightly. Can you please tell me more about what you did?

Comment 27

[Virginia Thayer (:thayerve)]

Sorry, I must've been doing something wrong with the test. Thanks for your help! Looks like this resolves the bug, can we mark it resolved?

Comment 28

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

Comment on attachment 8505087 [details] [diff] [review]
updatePatchInfo.patch

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

Ok, so lets continue on that one. The patch looks fine to me and as I mentioned earlier it also works. There are three things you might wanna do before it can be landed. Two of them you can find in my inline comment, and the other is to add a proper commit message, and yourself as author of the patch. See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests#The_review_process for help on that.

::: firefox/lib/software-update.js
@@ +304,5 @@
>     */
>    get patchInfo() {
> +    var info = {
> +      channel : this.updateChannel.channel
> +    }

nit: Please add a final semicolon at the end of the line.

::: firefox/tests/update/testFallbackUpdate/testUpdate.js
@@ +80,2 @@
>    persisted.updates[persisted.update.index].patch = update.patchInfo;
> +}

Something is off with the indentation of the code here. Please get this fixed.

Comment 29

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

Virginia, do you have an update for us? Thanks.

Comment 30

[Virginia Thayer (:thayerve)]

Attachment: Bug-897382: Include update channel in report even for failed update tests

Hello Henrik, apologies for the delay! I had to learn some more about Mercurial configuration to apply these changes, but I believe this patch reflects the changes you suggested. How does it look to you?

Comment 31

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

Comment on attachment 8517820 [details] [diff] [review]
Bug-897382: Include update channel in report even for failed update tests

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

Thank you for the update Virginia! The meta data for Mercurial indeed looks fine now. :) But the changes I have requested haven't been done. Did you miss a final qref?

::: firefox/lib/software-update.js
@@ +304,5 @@
>     */
>    get patchInfo() {
> +    var info = {
> +      channel : this.updateChannel.channel
> +    }

The missing final semicolon has not been added as pointed out in my last review.

::: firefox/tests/update/testDirectUpdate/testUpdate.js
@@ +77,5 @@
> +    update.download();
> +  }
> +  finally {
> +    // Store details about the patch
> +    console.log("It's me, Virginia!");

That's a nice Easter egg, but we don't really want that. :)

::: firefox/tests/update/testFallbackUpdate/testUpdate.js
@@ +80,2 @@
>    persisted.updates[persisted.update.index].patch = update.patchInfo;
> +}

The indentations have not been fixed as pointed out in my last review.

Comment 32

[Virginia Thayer (:thayerve)]

Whoops! That console.log was meant to help me test something at an earlier stage... I must not have saved the latest changes like I thought I had. Will have a new patch in the next couple days.

Comment 33

[Virginia Thayer (:thayerve)]

Attachment: updatePatchInfo.patch

Here is the correct patch I meant to upload the other day. All requested changes have been made, and no silly console.log! Let me know what you think.
Thanks!

Comment 34

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

Comment on attachment 8519261 [details] [diff] [review]
updatePatchInfo.patch

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

There are still 2 changes left. Please fix them and we are ready to get it landed.

::: firefox/lib/software-update.js
@@ +304,5 @@
>     */
>    get patchInfo() {
> +    var info = {
> +      channel : this.updateChannel.channel
> +    }

nit: This is still missing the trailing semicolon.

::: firefox/tests/update/testFallbackUpdate/testUpdate.js
@@ +82,1 @@
>    // Set the downloaded update into failed state

nit: while you have to do the other small fix, please keep the blank line above.

Comment 35

[Virginia Thayer (:thayerve)]

Attachment: updatePatchInfo.patch

Hi Henrik, after we talked on IRC the other day, I was able to fold my last two changesets into one diff and create the patch. This should include all necessary changes, including the semicolon and blank line I just added in. Hope this works!

Comment 36

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

Comment on attachment 8521744 [details] [diff] [review]
updatePatchInfo.patch

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

::: firefox/lib/software-update.js
@@ +304,5 @@
>     */
>    get patchInfo() {
> +    var info = {
> +      channel : this.updateChannel.channel;
> +    }

You cannot put a semicolon inside the block. This is causing an exception. So that's why it is important to test patches.

The semicolon has to put outside of the brackets to finish the declaration.

Comment 37

[Virginia Thayer (:thayerve)]

Attachment: updatePatchInfo.patch

Moved the semicolon as requested.

Comment 38

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

Comment on attachment 8523259 [details] [diff] [review]
updatePatchInfo.patch

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

That looks perfectly fine now. Thank you Virginia. I will get your patch landed now.

Comment 39

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

Landed on default as: https://hg.mozilla.org/qa/mozmill-tests/rev/cdeb5a66007b

If all is working well, I will get it backported by tomorrow.

Comment 40

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

So this patch works perfectly as we were able to see yesterday:
http://mozmill-daily.blargon7.com/#/update/report/635fcffb06b246be47416301bd4b0ef5

It's a minimal change so I will backport it straight to all the other branches:

https://hg.mozilla.org/qa/mozmill-tests/rev/adfe1e476e54 (aurora)
https://hg.mozilla.org/qa/mozmill-tests/rev/c92390b52896 (beta)
https://hg.mozilla.org/qa/mozmill-tests/rev/d61d5d3df289 (release)
https://hg.mozilla.org/qa/mozmill-tests/rev/9d1d24fe1c2e (esr31)

Virginia, thank you a lot for this patch. If you are interested to learn more while working on other bugs please let me know.

Comment 41

[Virginia Thayer (:thayerve)]

Thank you for all your help with my first code contribution, Henrik!

Comment 42

[Lukas Blakk [:lsblakk] use ?needinfo]

Yay Virginia! You rock!  http://i358.photobucket.com/albums/oo22/DroidWelder/Kermitflailing.gif