693306 - Refactor testrun scripts to minimize maintenance burden

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

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

The maintenance of all the existent automation scripts is kinda time invasive. We should condense all of those into a single script which accepts the type of test-run as parameter or option. We could even let the types be specified as list and execute all of those mentioned test-runs at once. That way we could even get rid of other scripts like testrun_all.py. Per default we could execute all of the test-runs, except updates which would cause way more test failures due to already installed latest builds.

Any further ideas or objections?

Comment 1

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

In terms of having a single entry point, I agree. Let's have one front-end script.

That said, if there are fundamental differences in the test runs, it might make sense to take a driver approach. 

Basically, take a command-line param that's the name of the run. That in turn translates into importing a module or object by that name that actually has the behavior of the run.

We write the objects or modules so they have a consistent interface (prep, run, cleanup being the likely main verbs) so they can be treated consistently by the front end script, but the objects contain the behavior differences. Use inheritance/delegation as appropriate to consolidate common operations.

Comment 2

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

(In reply to Geo Mealer [:geo] from comment #1)
> We write the objects or modules so they have a consistent interface (prep,
> run, cleanup being the likely main verbs) so they can be treated
> consistently by the front end script, but the objects contain the behavior
> differences. Use inheritance/delegation as appropriate to consolidate common
> operations.

Please check the lib/testrun.py module, which already contains all the available test-runs as classes. All of them work the same way as given by the base TestRun class. There will be a refactoring for the adoption to Mozmill 2, but that's not something I'm planning for now. It will be done later this or next month.

I would like to get this done before working on bug 693547.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

I'll take a look. So I hear you saying it already works more or less the way I suggested, we just need to unify them under a common front end? Sounds straightforward.

Comment 4

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

That's correct.

Comment 5

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

Attachment: Patch v1

New script which can now be used to trigger any existent testrun by just varying the --testrun option. This option is also a list, which means you can specify multiple testruns to execute.

I have not changed or removed existing scripts because those rely on Mozmill Crowd and our current integration on qa-horus for daily testruns.

I will update and release a new version of Mozmill Crowd once the patch has been checked-in. Once it has been approved and users have upgraded, we can get the old scripts removed.

For the testrun_daily and testrun_all I would like to move to a real CI first.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 572469 [details] [diff] [review]
Patch v1

I'm r+ing because the code's fine, but this isn't the direction I'd hoped you'd go and I really wish we'd confer on design before shooting ahead on solutions.

This could have been implemented without hardcoding the available testrun types by:

A) making the option take the unique part of the testrun class without validation, then importing the class with that name.

IOW, if I send in addons, it could just capitalize the first letter and attempt to import the AddonsTestRun class. If I send in foo, it would look for FooTestRun.

B) delegating the Option Groups to the class pulled in, as opposed to defining them here. So testrun.py --help would only give you high-level help, but testrun.py --testrun update --help would give you options for that testrun.

This breaks modularity pretty badly by making the entry point script have to know everything about the test classes, and would not be my preferred way to do this. 

I see this currently as not as good a solution as separate files because it'll present a monolithic list of options rather than options appropriate in context to what you're doing. There's value there in multiple, simpler entry points.

I'd at least like to see davehunt's feedback prior to your checkin.

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 572469 [details] [diff] [review]
Patch v1

Oh, one other point. 

--testrun needs to define its default of "functional" in the optparse specification, rather than checking for None later. So does anything else that fills in an default value later in a script. Otherwise the --help won't tell you what the default is. 

This is also an argument for delegating option groups to imported classes, since you don't want the master script to have to know all the defaults, etc.

Also, if there are any of these that aren't optional (i.e. don't have defaults), they need to be moved to positional parameters per command-line convention. We blow this all over the place (putting everything in --options) and it's really, really confusing; the difference between a positional and an --option is important.

If that's impossible w/ the single-entry point mechanism, that's a clear argument for not using single-entry point.

Comment 8

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

I like Geo's suggestion of dynamically importing the class based on the value of the --testrun option, but I guess I don't feel strongly either way.

As for folding the testrun specific options in help, I'm not sure if this will be obvious to users if the --testrun is an optional argument. I'd perhaps pick up on Geo's final point to suggest that the testrun is not optional, but a required first argument.

testrun.py --help
...shared help

testrun.py update --help
...shared help + update testrun help

Comment 9

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

I guess I'm a little concerned that we're trading what seems like a minimal maintenance issue on separate scripts for a maintenance chokepoint with less usability. 

I know I said "lets go for it" in c2, but that was before I really grokked that everything would need separate command line signatures. If I'd understood that, I'd have suggested just refactoring to move as much shared code into libraries as possible, so each script became little other than OptParse + a class initialization/run/cleanup.

Imagine if these were functions instead of scripts, and the command line was the function call instead. Would we really want to combine all the functions and have a monster function call signature with all options accounted for? 

Probably not; you'd want smaller, simpler functions so that each one was easier to maintain, and have them share code where necessary to keep DRI. This is the same thing.

Comment 10

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

(In reply to Geo Mealer [:geo] from comment #6)
> I'm r+ing because the code's fine, but this isn't the direction I'd hoped
> you'd go and I really wish we'd confer on design before shooting ahead on
> solutions.

See my comment below on that, because there are restriction you probably haven't thought about.

> A) making the option take the unique part of the testrun class without
> validation, then importing the class with that name.

So when you don't want to hard-code the possible choices, how will the user know about values they will be able to use? I do not want to move this into the help attribute of the parser option, because it doesn't belong there and will not give that detailed parser error if a wrong value has been used. I kinda would like to fallback to already existent code. Further you will have to update the list in any way, if new testruns get added. Otherwise I can only see us raising an exception when trying to instantiate an unknown class.

> IOW, if I send in addons, it could just capitalize the first letter and
> attempt to import the AddonsTestRun class. If I send in foo, it would look
> for FooTestRun.

That's something we could do to get rid of the mapping constant. That's right and a good idea. Probably I can even find a way to automatically set the list of testrun options by retrieving the classes from the lib/testrun.py file.

> B) delegating the Option Groups to the class pulled in, as opposed to
> defining them here. So testrun.py --help would only give you high-level
> help, but testrun.py --testrun update --help would give you options for that
> testrun.

So tell me how this is doable. While its a nice approach I really don't see a way how to fulfill that. The parser options and args are getting parsed before any of the testrun classes get instantiated. Not sure how a help page could be shown at a later time.

> I see this currently as not as good a solution as separate files because
> it'll present a monolithic list of options rather than options appropriate
> in context to what you're doing. There's value there in multiple, simpler
> entry points.

It's a hell of maintainance to get all different kind of scripts updated with new options, and nearly 100% code duplication. With the single entry point you can now directly define an ordered list of test-runs to execute. Before you have had to do some additional scripting yourself to make it possible. See testrun_all.py which I would really like to kill.

(In reply to Geo Mealer [:geo] from comment #7)
> --testrun needs to define its default of "functional" in the optparse
> specification, rather than checking for None later. So does anything else
> that fills in an default value later in a script. Otherwise the --help won't
> tell you what the default is. 

--help won't tell you that in any way. Or I haven't found a way yet to force the display of default values. Otherwise I will update the code. There was a problem with setting the default before and I simply forgot to revert that. Thanks for noticing that.

(In reply to Geo Mealer [:geo] from comment #9)
> I know I said "lets go for it" in c2, but that was before I really grokked
> that everything would need separate command line signatures. If I'd
> understood that, I'd have suggested just refactoring to move as much shared
> code into libraries as possible, so each script became little other than
> OptParse + a class initialization/run/cleanup.

In terms of feedback I would really appreciate if we all would take a look at the code first. We all have to know about it in any way. Those are valid points but in the future I kinda would like to get those earlier.

I will check tomorrow how to make that better to use and how to better organize the code.

Comment 11

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Henrik Skupin (:whimboo) from comment #10)

> So when you don't want to hard-code the possible choices, how will the user
> know about values they will be able to use? I do not want to move this into

> a way how to fulfill that. The parser options and args are getting parsed
> before any of the testrun classes get instantiated. Not sure how a help page
> could be shown at a later time.

Just noting that these are problems created by moving to a single uberscript. In separate scripts, these issues don't exist.
> > A) making the option take the unique part of the testrun class without
> > validation, then importing the class with that name.
> 
> So when you don't want to hard-code the possible choices, how will the user
> know about values they will be able to use? I do not want to move this into
> the help attribute of the parser option, because it doesn't belong there and
> will not give that detailed parser error if a wrong value has been used. I

OK, well, asked and answered I guess. I hear you re: detailed error. It's a good point, though I'm unsure whether I'd choose that side of the tradeoff. 

Do you feel users will be calling this directly much?

> right and a good idea. Probably I can even find a way to automatically set
> the list of testrun options by retrieving the classes from the
> lib/testrun.py file

> 
> > B) delegating the Option Groups to the class pulled in, as opposed to
> > defining them here. So testrun.py --help would only give you high-level
> > help, but testrun.py --testrun update --help would give you options for that
> > testrun.
> 
> So tell me how this is doable. While its a nice approach I really don't see
> a way how to fulfill that. The parser options and args are getting parsed
> before any of the testrun classes get instantiated. Not sure how a help page
> could be shown at a later time.

Easy: don't make the testrun type an --option, and make it a positional parameter instead. 

There's no particular reason it should be defaulting to functional anyway, in a run-everything script (example, we run more update tests than functional tests per release, so the logical default is update, which doesn't seem right either). If it has no default, making it positional is correct. 

So then you use it to instantiate the class, which gives you back the appropriate tuples to instantiate the parser, and you do the rest of the stuff from there.

> It's a hell of maintainance to get all different kind of scripts updated
> with new options, and nearly 100% code duplication. With the single entry
> point you can now directly define an ordered list of test-runs to execute.
> Before you have had to do some additional scripting yourself to make it
> possible. See testrun_all.py which I would really like to kill.

It's not always a win to combine similar code; really depends on whether it's truly the same code, and whether it causes cohesion issues by forcing you to combine modules that shouldn't be combined. I think this causes cohesion issues, which is where I'm coming from.

Can you give an example of the options we've added that apply to -all- scripts we might ever run with this? Usually those get set pretty quickly. If it only applies to one or two scripts, this isn't a maintenance win.

> --help won't tell you that in any way. Or I haven't found a way yet to force
> the display of default values. Otherwise I will update the code. There was a
> problem with setting the default before and I simply forgot to revert that.
> Thanks for noticing that.

Think we went through the default value thing on my scripts, which show them correctly. From optparse docs:

parser.add_option("-m", "--mode",
                  default="intermediate",
                  help="interaction mode: novice, intermediate, "
                       "or expert [default: %default]")

You have to actually define the default in the option spec, then use %default in the string and it'll be substituted.

> In terms of feedback I would really appreciate if we all would take a look
> at the code first. We all have to know about it in any way. Those are valid
> points but in the future I kinda would like to get those earlier.

I assume you mean original code? I think that if we'd had a conversation that came down to "these are the specific situations that have been maintenance problems, and here's what the interface for the new script would look like" this wouldn't be a surprise. The first chance I've had to see the script interface was on delivery.

It's a common theme: script interfaces, object interfaces, etc. I don't really care all that much about how stuff's implemented if it's not completely out there, but interfaces and modularity are what define usability and future-proofing.

I'm always in favor of defining interfaces first, reviewing them, then writing the code to fulfill them. If you want me to ask for that specifically, I'd be happy to do so.

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Oh, didn't address but wanted to say the ability to define a test list is good. But it's the type of thing where I'd typically use a shell script or small Python wrapper that called the individual test scripts. 

OTOH, if I'm not mistaken that's where we're at now and you don't like maintenance on that.

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

This has stalled a bit. 

If you feel strongly it's the right way to go, you do have my r+ and Dave's feedback, and I won't object to you checking in--mostly because I don't anticipate humans dealing with this script much at all once we have the framework.

That said, I stand by my opinions on cohesion and UX. If this were going to be primarily user-driven, I'd probably feel differently about it being OK to check in.

Comment 14

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

No, I don't want to land it as it is right now. You gave some good points and I kinda would like to address those first, or find a better solution. I will do some quick tests and will request feedback on it.

Comment 15

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

As we have seen a single entry point script is not the best way to go. Instead we should keep the individual scripts and move the optparser handling into our TestRun class for easier maintenance. I talked with Geo about it yesterday and he kinda liked my proposal. I will come up with a patch soonish.

Comment 16

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

Attachment: Patch v2

This version of the patch implements what has been discussed before. I have removed the single testrun entry script, and left all the existing testrun scripts in the folder. The really big improvement here is that all optparse code has been moved into the individual TestRun classes. That way no CLI script has to redeclare those options again and again. Further all arguments can now be specified by the class constructor. If none have been given we automatically fallback to sys.vargs. That allows additional wrapper scripts like the testrun_daily.py file to pass in custom arguments.

We are still a bit limited in handling optparse code because of the implementation in Mozmill, but that's fixable once we upgrade to Mozmill 2. The latter is able to handle OptionGroups and also doesn't always rely on sys.vargs.

Comment 17

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

Also I have removed the display code from the daily testrun script, because that really belongs to the crontab entry. Beside that I have used the current implementation of handling optparse because of the aforementioned dependency on Mozmill 1.5.x. We could do it better but staying in sync should be a higher priority.

Comment 18

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

Comment on attachment 575443 [details] [diff] [review]
Patch v2

>+class EnduranceTestRun(TestRun):
>+    """ Class to execute a Firefox endurance test-run """
>+
>+    report_type = "firefox-endurance"
>+    report_version = "1.2"
>+
>+    parser_options = copy.copy(TestRun.parser_options)
>+    parser_options[("--no-fallback",)] = dict(dest="no_fallback",
>+                                              action="store_true",
>+                                              default=False,
>+                                              help="Do not perform a fallback update")

--no-fallback is not an endurance testrun option

Comment 19

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

Attachment: Patch v2.1

Thanks Dave for noticing that. Right, I simply forgot to remove those lines after the c&p and editing action. Patch updated.

Comment 20

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 575870 [details] [diff] [review]
Patch v2.1

Code looks fine. I like having the options delegated like that. Much cleaner, thanks! r+

Comment 21

[David Burns :automatedtester]

Comment on attachment 575870 [details] [diff] [review]
Patch v2.1

looks good to me

Comment 22

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

Thanks everyone for the feedback. It was very valuable and turned us into the right direction.

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

Comment 23

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 575870 [details] [diff] [review]
Patch v2.1

review above was this version, think the r? got flipped back by mistake. Returning to r+ me.

Comment 24

[u279076]

This bug has completely broken our listener infrastructure.

release_push.py expects testrun_all.py which was removed. Same goes for release_bft.py. How do I execute the automation now? This is a blocker for Firefox testing which is supposed to be happening right now.

Comment 25

[u279076]

David had advised me to back out the changeset due to Henrik being on PTO and Geo being out sick. I attempted this but was faced with a merge conflict I was not confident in resolving.

For the purposes of testing, I am checking out an older revision to a sandbox folder on the servers.

Reopening this bug so the patch can include changes to necessary release_*.py scripts.

Comment 26

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

I think the removal of testrun_all.py was ultimately unintentional. I believe it was part of the initial patch, and may have been forgotten to reinstate it. I tried however to reintroduce it and got an error regarding 'logfile' being an unexpected keyword argument when attempting to run release_bft.py.

Comment 27

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

FWIW, release_push.py only has a dependency on release_bft.py (or whatever script you point it at, since you can change that default). Fix the release_bft.py issue, and the listener setup will work.

Comment 28

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

As said by Geo, this problem has been caused by the release_bft.py script. I simply forgot to revert the change I have made earlier to test how long a cycle with all testruns takes. Because we never agreed yet, which testruns we want to run, I have safely reverted the call to testrun_functional only. Once we have the CI system setup we can discuss which additional testruns we want to execute for releases.

All machines for testing releases have been updated. Closing as fixed again.