Closed Bug 1195195 Opened 9 years ago Closed 9 years ago

mozregression.cli should have a small and well defined public interface

Categories

(Testing :: mozregression, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: parkouss, Assigned: parkouss)

References

Details

Attachments

(1 file)

Once bug 1195082 will be resolved, we will have a new cli module.

Fine, but a lot of stuff in there will be needed by the main module, and we should try to not do such tight coupling.

I suggest that we create a single entry point, a *cli* method that will create and return a Configuration object with three public properties on it:

 - options: the namespace of options values extracted from the command line, and possibly modified by default values or other stuff from inside cli call only.
 - fetch_config: the created fetch_config instance. fetch_config is our base for bisection configuration, and we have cli checks that rely on that. So let's create it from cli() call and make it available via the configuration object.
 - action: this is a string representing the action that the user want to do. we can only decide that when parsing command line. Currently this should be either 'bisect_nightlies' or 'bisect_inbounds', but we can think about a 'launch' future value (bug 1194941).

The cli() function should do everything to check the values given by command line.

With that, the main module will be easy to write. the logic should then be:

> try:
>     config = cli()
> except MozRegressionError, exc:
>     sys.exit("Error: " % exc)
> 
> # configuration is all good here. we have our options, our fetch_config
> # and the action name that we need to take. So now do things depending
> # on the action value, that's all.

This will make things a lot easier, by abstracting the cli "dirty" work needed.


Well this is a proposal. This is totally open to discussion, feel free to say something if you have ideas!
Well, I'm sorry this is quite a big review. This basically move code and wrap it in the cli module as described above. Some changes from the proposal:

 - the configuration object also holds a logger, and a test_runner.
 - the configuration object needs a two step validation, so there is a method validate()

So I tested this basically, it seems to work well. The idea behind all these changes is that it will be easier to integrate the --launch argument. This is how I see things:

1. create a Configuration object. This parse the command line, check and fix arguments, and create at minima the fetch_config. (for now I put the test runner, but I am not really sure yet for this one).

2. create an application. this will be initialized from the configuration, and will be responsible to create what is needed to run the application (bisecting inbounds, nightly, or just running a build). The application is currently what is called "bisect runner". I intend to rename that to Application, and extend it to support running a single build.
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8650439 - Flags: review?(wlachance)
Comment on attachment 8650439 [details] [review]
mozregression.cli should have a small and well defined public interface

I mostly skimmed through this, but the basic ideas look right to me.
Attachment #8650439 - Flags: review?(wlachance) → review+
Thanks Will - should be cleaner from the main interface now, even if it is not perfect. :)

I fixed the nits you found, great!

Landed in https://github.com/mozilla/mozregression/commit/781e4de7427f7b4191a448da27c9f98f20dd3b49.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: