643473 - endTest passes redundant data -- passes and passed

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

[Jeff Hammel]

frame.js passes both the passes and fails as well as the length of the
passes and fails: 

https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L192

This should be done at the event-consumer level, not the event-sender level

Comment 1

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

The location of the code has been changed. It's available now here:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L200

Comment 2

[Sambuddha Basu [:sambuddhabasu1]]

I am interested in solving this bug. Can someone assign it to me?

Comment 3

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

Sure. It's great to see that you have interest to work on it. I love to see it fixed. Thanks in advance!

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Hey Sambuddha, I am checking in with this bug to see if you have all the information you need.  Feel free to ask in the bug if you get stuck or need help.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:whimboo, can you outline what needs to be done here?  I am reading the bug in more details and really have no idea what to do.  I would like to know:
* what parameters to include in https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L200
* what other code references this and should be modified
* what tests need to be written
* other caveats that might make this a difficult patch to r+

Comment 6

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

Well, it's just this code here as outlined above:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L197

So the length of both objects has to be computed by the event consumer and not the sender.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

so we should remove those two variables and do nothing else?  Any tests to write/update?  Any consumers that call or depend on this that need to be updated?

Without clear information this shouldn't be a good first bug, it could be a "good next bug" though.  I think this is a simple bug, just lacking some more well defined instructions.

Comment 8

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

As the whiteboard says js and py is involved. So the JS side is the event-producer and the Pyhton side the event-consumer. As comment 0 says, the consumer (here the endTest listener) should set those properties.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

can you link to the python consumer?

Comment 10

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

Here the link to the consumer:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__.py#L73

Comment 11

[Sambuddha Basu [:sambuddhabasu1]]

Hey Henrik, it would be great if you could assign this bug to me.

Comment 12

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: endTest passes redundant data -- passes and passed

Comment 13

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

Comment on attachment 8481341 [details] [diff] [review]
endTest passes redundant data -- passes and passed

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

::: mozmill/mozmill/__init__.py
@@ -414,5 @@
>  
>                      # see frame.js:events.endTest
>                      obj = {'filename': test['path'],
> -                           'passed': 0,
> -                           'failed': 0,

The removal here is fine, but you also have to update the code inside the endTest listener as stated in one of my comments before.

Also please check for other instances. Under report_disconnect() we have the same construct.

::: mozmill/mozmill/logger.py
@@ +239,5 @@
>  
>      def endTest(self, test):
>          filename = self.mozmill.running_test.get('relpath', test['filename'])
> +        test['passed'] = len(test['passes'])
> +        test['failed'] = len(test['fails'])

Those lines you will have to add to the endTest listener in __init__.py. Here it is not necessary. For the one length check below you can directly use the above without a temporary variable.

Comment 14

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

Andrei, I wonder if we completely could get rid of these properties. I know that we would have to update the dashboard code to not base on the passed and failed properties. Do you think its worth the time? Otherwise we will leave it and wait which changes we would have to do for treeherder. I mostly would favor that.

Comment 15

[Andrei Eftimie]

I would still leave them for now. mozmill-dashboard is already slow when showing a result, this would add more overhead in showing a result.

(Not sure if it would be significant or not, we would have to do some timing checks).

I agree we should wait for treeherder since we will most likely need to overhaul most of the report code for it.

Comment 16

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

Mozmill will reach its end of life soon. We are currently working on getting
all the tests for Firefox ported to Marionette. For status updates please see
bug 1080766.