564957 - Allow classes derived from MozMill to add/modify/remove report data and to set its own test type

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

Right now sub classes of MozMill cannot set their own test type without having to completely overwrite the get_report function. I tried to remove the get_report function from MozMillRestart and replaced the hard-coded test type strings with properties but then I always get the following error:

(9, 'Bad file descriptor')


This fix is needed for our release testing work, for add-on tests, l10n tests, and software update tests. All those tests need their own type.

Comment 1

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

Derived classes should also be able to add, modify, and remove result data to match the data which is needed for the dashboard.

Comment 2

[Heather Arthur [:harth]]

http://github.com/harthur/mozmill/tree/bug-564957.

you can specify self.test_type and get_result will use this.

Comment 3

[Heather Arthur [:harth]]

oh, forgot the add/modify data part, let me look into that

Comment 4

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

(In reply to comment #2)
> http://github.com/harthur/mozmill/tree/bug-564957.

I tried the same but it didn't worked for me. It hasn't used the class member which I have specified the same way as you did here. Have you tested it by sending the result to a couchdb instance?

Comment 5

[Heather Arthur [:harth]]

the "(9, 'Bad file descriptor')" is unrelated, I get it whenever I specify a server to report to (with or without the code changes). I think it's because self.bridge is not defined by the restart class because restart overrides the start method but does not call create_network (which defines self.bridge). The full exception is:

Traceback (most recent call last):
  File "/usr/local/bin/mozmill-restart", line 8, in <module>
    load_entry_point('mozmill==1.4.1', 'console_scripts', 'mozmill-restart')()
  File "/Users/harth/mozmill/mozmill/restart.py", line 228, in restart_cli
    RestartCLI().run()
  File "/Users/harth/mozmill/mozmill/restart.py", line 225, in run
    super(RestartCLI, self).run(*args, **kwargs)
  File "/Users/harth/mozmill/mozmill/mozmill.py", line 390, in run
    self._run()
  File "/Users/harth/mozmill/mozmill/mozmill.py", line 361, in _run
    self.options.report)
  File "/Users/harth/mozmill/mozmill/restart.py", line 202, in run_tests
    results = self.get_report(test_dir, starttime, endtime)
  File "/Users/harth/mozmill/mozmill/mozmill.py", line 261, in get_report
    app_info = self.get_appinfo(self.bridge)
  File "/Users/harth/mozmill/mozmill/mozmill.py", line 233, in get_appinfo
    mozmill = jsbridge.JSObject(bridge, mozmillModuleJs)
  File "/Library/Python/2.6/site-packages/jsbridge-2.3.5-py2.6.egg/jsbridge/jsobjects.py", line 76, in __init__
    name = bridge.set(name)['data']
  File "/Library/Python/2.6/site-packages/jsbridge-2.3.5-py2.6.egg/jsbridge/network.py", line 214, in set
    return self.run(_uuid, 'bridge.set('+', '.join([encoder.encode(_uuid), obj_name])+')')
  File "/Library/Python/2.6/site-packages/jsbridge-2.3.5-py2.6.egg/jsbridge/network.py", line 183, in run
    self.send(exec_string)
  File "/Library/Python/2.6/site-packages/jsbridge-2.3.5-py2.6.egg/jsbridge/network.py", line 81, in send
    asyncore.dispatcher.send(self, b)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/asyncore.py", line 349, in send
    result = self.socket.send(data)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/socket.py", line 165, in _dummy
    raise error(EBADF, 'Bad file descriptor')
socket.error: [Errno 9] Bad file descriptor


Why do you need to override the start method? (just wondering).

I updated my code to deal with the subclass adding/modifying data (not removing it). http://github.com/harthur/mozmill/commit/4b547fda2c0792e5c23ae8555fbc4966c1ee45ec. This adds a get_report_additions method that the subclass can override.

The other option is calling super from the subclass and in this case you can add/modify/remove whatever you want: http://github.com/harthur/mozmill/compare/seperate-restart...bug-564957-2

Comment 6

[Heather Arthur [:harth]]

hm, my bad, MozmillRestart does call create_network. Henrik, I'm using this command to run the restart tests:

mozmill-restart -t mozmill-tests/firefox/restartTests/testMasterPassword/ --report=http://127.0.0.1:5984/mozmill

and I get that 'Bad file descriptor' exception. Have you been able to run them without getting that exception?

Comment 7

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

(In reply to comment #6)
> hm, my bad, MozmillRestart does call create_network. Henrik, I'm using this
> command to run the restart tests:
> 
> mozmill-restart -t mozmill-tests/firefox/restartTests/testMasterPassword/
> --report=http://127.0.0.1:5984/mozmill

The URL is wrong. Please try with this URL and the correct design docs installed:
http://127.0.0.1:5984/mozmill/_design/reports/_update/report

> and I get that 'Bad file descriptor' exception. Have you been able to run them
> without getting that exception?

Does that solve the problem?

Comment 8

[Heather Arthur [:harth]]

I don't think that part matters, it just sends a POST request to whatever url you give it. just doing 'mozmill --report=http://localhost:5984/mozmill' works.

Is this a regression? Does the restart reporting work for you?

Comment 9

[Heather Arthur [:harth]]

okay, the problem is that the jsbridge socket was closing before get_appinfo was being called in mozmill's get_report. A temp fix is to just call assign self.appinfo earlier:

http://github.com/harthur/mozmill/compare/bug-564957-3

Comment 10

[Heather Arthur [:harth]]

Henrik, can you review these:

http://github.com/harthur/mozmill/commit/cd7b141de84ef01918cd3a9d0bead733ba5797d0
http://github.com/harthur/mozmill/commit/c16a6277596921fb79fea4ab541284f42ede9f4c
http://github.com/harthur/mozmill/commit/cf7413265cce6db6820e61ba741dae02c438bbf4
http://github.com/harthur/mozmill/commit/27eeefb695b4b63563ca9283dc3e72d0d0f3240e

I like using super for this case because you can do whatever you want to the report afterwards. If, however, we're just going to modify the 'type', having a self.test_type property would be nicer.

Comment 11

[Heather Arthur [:harth]]

Jeff suggests using MozMill class explicitly instead of 'super':
http://github.com/harthur/mozmill/commit/558a4070f4c16ec628b6e51fcd8bc3b0a776c31d

Comment 12

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

Ok, so the current state is not that perfect because it requires that all derived classes have to set the test, starttime, and endtime parameters when they override the get_report function. We definitely shouldn't force classes to set those values. All that should only be done by the base class.

Can we remove all the parameters and fold them in via self.test, self.starttime, and self.endtime. I'm even not sure why we are relying so much on parameters for functions.

Comment 13

[Heather Arthur [:harth]]

true. Though I don't think these are good class properties, semantically. They make more sense as arguments. I vote for adding a self.test_type property and using that from the superclass's get_report.

Are there any current projects you're working on that would need to add or remove report data before sending off?

Comment 14

[Heather Arthur [:harth]]

mm, I forgot, we can use just say:

def get_report(self, *args, **kwargs):
  report = Mozmill.get_report(self, *args, **kwargs)
  # add/modify/remove stuff with report

if we need to. For right now, I think the self.test_type thing is nice, what do you think Henrik?

Comment 15

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

(In reply to comment #13)
> Are there any current projects you're working on that would need to add or
> remove report data before sending off?

Most of the test-run scripts you can find in the repo below will have to make use of it: http://hg.mozilla.org/qa/mozmill-automation/file/tip

(In reply to comment #14)
> mm, I forgot, we can use just say:
> 
> def get_report(self, *args, **kwargs):
>   report = Mozmill.get_report(self, *args, **kwargs)
>   # add/modify/remove stuff with report
> 
> if we need to. For right now, I think the self.test_type thing is nice, what do
> you think Henrik?

That sounds great. So I would have to set self.test_type before I call Mozmill.get_report or can it also happen afterward. When will that value be stored in the report itself?

Comment 16

[Heather Arthur [:harth]]

alright, I propose these changes to Mozmill:

http://github.com/harthur/mozmill/compare/reporttype

and using the Mozmill.get_report(self, *args, **kwargs) business for heavier duty report data modifying from subclasses.

Comment 17

[Heather Arthur [:harth]]

(In reply to comment #15)
> > For right now, I think the self.test_type thing is nice, what do
> > you think Henrik?
> 
> That sounds great. So I would have to set self.test_type before I call
> Mozmill.get_report or can it also happen afterward. When will that value be
> stored in the report itself?

I think just having it as a class attribute works, we're not going to be changing it. then get_report can fetch it.

Comment 18

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

Merged with master:
http://github.com/mikeal/mozmill/commit/485016a81e670565d9fa3c99f6e0f90efd8ddd1a

Thanks Heather!

Comment 19

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

Verified fixed by running the dev build of Mozmill and my locally updated testrun classes.