692793 - Add ability to specify number of cycles in talos test

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[William Lachance (:wlach)]

For testing purposes, it would be useful to occasionally be able to specify the number of cycles that a test should be run in Talos, instead of always defaulting to 10.

(I know I could get this behaviour by creating a new template too, but adding a command line option would make this even easier)

Comment 1

[William Lachance (:wlach)]

Attachment: Add ability to specify number of cycles in talos tests

This patch also has a minor change to make it easier to add/view PerfConfigurator attributes.

Comment 2

[William Lachance (:wlach)]

Attachment: Updated patch to specify number of cycles

I realized that the previous patch won't do, as it specifies a default number of cycles (10) which we always use even when we don't want that in all cases (e.g. tp runs for a default of 1 cycle).

This new patch is smarter: it'll only overwrite the # of cycles if the user explicitly specifies it on the command line. It also takes more care to only replace lines of the form "cycles: 1234" and preserve their exact amount of whitespace.

Comment 3

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

Comment on attachment 565740 [details] [diff] [review]
Updated patch to specify number of cycles

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

Overall I don't like this patch.  The idea of --cycles shouldn't be variable.  Some tests have issues (specifically on mobile) if we increase the cycles, this is why tp4m isn't 10 cycles.  In addition we have -tpcycles on the pageloader command line which isn't being modified in this patch.

Can you give me examples of how this is useful?  Personally I just change it in my .yml file before running the test if I need to change it.  That is just as much work as changing a commandline argument.

::: PerfConfigurator.py
@@ +201,5 @@
>                  #if noChrome is True remove --tpchrome option 
>                  newline = line.replace('-tpchrome ','')
>  
> +            if self.cycles and re.match('^\s*cycles\s*:\s*[0-9]+\s*\n', line):
> +                newline = re.sub('\s*:.*\n', ': '+self.cycles+'\n', line)

Two things here:
1) verify self.cycles is a valid value (>0, <100 and an integer)
2) re.sub('\s*:.*\n', ': '+self.cycles+'\n', line) should be:
re.sub('\s*:.*\n', ': %s\n' % self.cycles, line)

Comment 4

[William Lachance (:wlach)]

Yeah, the more I think about it the more I think this shouldn't go in. It's just too fragile and weird relative to the benefit.

The main motivation for writing this in the first place was that I've frequently wanted to just quickly see if talos can just "do" a test, and editing the yml file isn't my favored way of doing that (since if I re-run the configurator my changes will be blown away). Maybe there's a better way of doing this though. How about a "test mode" for talos that will only run things once (regardless of the # of cycles specified in the yml)?

Comment 6

[William Lachance (:wlach)]

As discussed above, let's not fix this. We can file a seperate bug about a test mode at some later point.