Talos config files should permit a generic "tests" section that the other tests inherit from

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: k0scist, Assigned: jmaher)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Currently, each test suite in talos has a bunch of information in
e.g. sample.config that is repeated for each set of tests:

http://hg.mozilla.org/build/talos/file/4887e3aebe37/talos/sample.config#l99

Instead, there should be a generic section that *all* tests inherit
from, but that individual test suites can override
(Assignee)

Comment 1

7 years ago
Created attachment 589961 [details] [diff] [review]
create a basetest which has default values (1.0)

as discussed briefly in IRC, we should have a much simpler manifest solution and the first step is to create a base class.  I went with a new section vs another test inside of tests.  

I am not sure if we want this in run_tests.py, but my looking at PerfConfigurator.py seemed as though it would be a lot of work.  This is a first stab, if we like it great, if not, back to the drawing board.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #589961 - Flags: review?(jhammel)
Attachment #589961 - Flags: feedback?(chmanchester)
(Reporter)

Comment 2

7 years ago
Comment on attachment 589961 [details] [diff] [review]
create a basetest which has default values (1.0)

+# Prototype function, this should be in PerfConfigurator, so our resulting .yml file has a single bit  

Not really sure what this means, nor am I convinced in the long term that it should be in PerfConfigurator, as I would ideally like to combine the configuration stuff from PerfConfigurator and run_tests (https://bugzilla.mozilla.org/show_bug.cgi?id=704654)

The function itself looks fine.

The rest of the patch looks fine.  I'm happy of any patch that deletes more code than it adds and provides the same functionality. We will need a follow-up for being able to modify parameters of basetest directly, but I'm happy to take that as a subsequent step.

Also, testbase vs. basetest?  I don't really care, tbh, but will go with popular opinion.
Attachment #589961 - Flags: review?(jhammel) → review+
Comment on attachment 589961 [details] [diff] [review]
create a basetest which has default values (1.0)

This looks great to me. One thought - I know we're planning to clean up/consolidate many of the .config files, but in the mean time, will this be problematic if a config file other than sample.config is specified to PerfConfigurator?

In terms of putting this in PrefConfigurator vs. run_tests, I'm with Jeff that these should be combined in the not too distant future.

For naming, it hardly matters to me, but basetest seems a little clearer ("base" as a modifier for "test").
(Assignee)

Comment 4

7 years ago
Created attachment 590089 [details] [diff] [review]
create a basetest which has default values (2.0)

tested with other config files as well as commandline options.  This seems to work well, doing a staging run overnight;
Attachment #589961 - Attachment is obsolete: true
Attachment #589961 - Flags: feedback?(chmanchester)
Attachment #590089 - Flags: review?(jhammel)
(Reporter)

Comment 5

7 years ago
Comment on attachment 590089 [details] [diff] [review]
create a basetest which has default values (2.0)

+  tests = useBaseTestDefaults(yaml_config.get('basetest', []), tests)

I probably should have caught this before, but shouldn't this be yaml_config.get('basetest', {}) ?

+# Prototype function, this should be in PerfConfigurator, so our resulting .yml file has a single bit
Again, not sure if this comment is necessary

Other than those, looks good!
Attachment #590089 - Flags: review?(jhammel) → review+
(Assignee)

Comment 6

7 years ago
http://hg.mozilla.org/build/talos/rev/0ee2d31e29c8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.