802937 - Allow filtering tests by prettyName substring tests

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Steve Fink [:sfink] [:s:]]

I don't know if this is a good idea or not. I was mostly just experimenting a little.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Allow filtering tests by prettyName substring tests

With this, you could for example do 

  try: -b do -p macosx,macosx64 -u mochitests[10.7]

and it wouldn't do any tests on OSX 10.6 or 10.8. Or

  try: -b do -p macosx,macosx64 -u mochitests[10.7,10.8],reftests[10.6]

if you just want to mess around. (Note that I don't understand what macosx vs macosx64 maps to, so I'm just throwing them both in.) It works by doing a substring match on the "prettyName".

Whether this is worth the complexity, I dunno.

Oddly, it probably also makes |-u all[mochi]| into an alias for |-u mochitests|.

Comment 2

[[e-mail] (:bhearsum)]

Just making sure I understand this correctly. The current state of affairs is that we can't do different test suites on different platforms within the same push. So if we wanted to build macosx and macosx64, and run mochitests on both and reftests on only one of them, we'd have to run reftests on both. With this patch, we can be more precise and only run reftests where we want them.

And as for implementation...for each enabled platform (eg, macosx), you iterate over the list of test suites. If the suite has no square brackets, it's enabled. If it has square brackets, iterate over the test platforms for the platform (eg, Rev4 MacOSX Snow Leopard 10.6), and if any of the comma-separated substrings inside the square brackets match the test platform, the suite is enabled. If not, it's disabled.

I have a couple of comments about the patch itself, but I just want to make sure I understand this correctly before giving it a full review. Sorry for the delay, and thank you for spending time on this!

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Ben Hearsum [:bhearsum] from comment #2)
> Just making sure I understand this correctly. The current state of affairs
> is that we can't do different test suites on different platforms within the
> same push. So if we wanted to build macosx and macosx64, and run mochitests
> on both and reftests on only one of them, we'd have to run reftests on both.
> With this patch, we can be more precise and only run reftests where we want
> them.

This is all true, but it's not the main point.

The specific problem I was addressing is that if you request a platform (eg macosx64) that corresponds to a single build but multiple test "sub-platforms" (eg osx 10.6, 10.7, and 10.8), currently there's no way to avoid getting a largely redundant set of tests on each of those sub-platforms.

So try: -b o -p macosx64 -u mochitests results in the tbpl output of

  OS X 10.6 opt M(1 2 3 4 5 bc oth)
  OS X 10.7 opt B M(1 2 3 4 5 bc oth)
  OS X 10.8 opt M(1 2 3 4 5 bc oth)

You get one build, on 10.7, and three full sets of tests. Currently, there's no way to make it give you just one set of tests. With this patch, you could do try: -b o -p macosx64 -u mochitests[10.7] to get:

  OS X 10.7 opt B M(1 2 3 4 5 bc oth)

or try: -b o -p macosx64 -u mochitests[10.8] to get:

  OS X 10.7 opt B
  OS X 10.8 opt M(1 2 3 4 5 bc oth)

(because our builders are all 10.7, I guess.)

> And as for implementation...for each enabled platform (eg, macosx), you
> iterate over the list of test suites. If the suite has no square brackets,
> it's enabled. If it has square brackets, iterate over the test platforms for
> the platform (eg, Rev4 MacOSX Snow Leopard 10.6), and if any of the
> comma-separated substrings inside the square brackets match the test
> platform, the suite is enabled. If not, it's disabled.

Yes. So no square brackets means "no restrictions". Square brackets mean "only if one of the comma-separated substrings matches".

Guess I should've commented it a bit.

> I have a couple of comments about the patch itself, but I just want to make
> sure I understand this correctly before giving it a full review. Sorry for
> the delay, and thank you for spending time on this!

Like I said, I'm not even convinced this is that great of an idea, but we need *something* to make it possible to avoid the redundant tests. And if you do think this is worth adding, then I wonder if it should also allow [-a,b,c] to mean everything *but* jobs matching a, b, or c. That would allow eg -u all[-mochi] to get all unit test suites except for mochitests ("I'm pushing a patch that I know doesn't affect mochitests, but I don't know what all the other tests do so give me all of the rest so I don't need to figure it out.")

Which points out that I probably ought to make the substring match case-insensitive, since the prettyNames might capitalize things randomly.

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 672656 [details] [diff] [review]
Allow filtering tests by prettyName substring tests

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

Thanks for the detailed explanation.

(In reply to Steve Fink [:sfink] from comment #3)
> (In reply to Ben Hearsum [:bhearsum] from comment #2)
> > Just making sure I understand this correctly. The current state of affairs
> > is that we can't do different test suites on different platforms within the
> > same push. So if we wanted to build macosx and macosx64, and run mochitests
> > on both and reftests on only one of them, we'd have to run reftests on both.
> > With this patch, we can be more precise and only run reftests where we want
> > them.
> 
> This is all true, but it's not the main point.
> 
> The specific problem I was addressing is that if you request a platform (eg
> macosx64) that corresponds to a single build but multiple test
> "sub-platforms" (eg osx 10.6, 10.7, and 10.8), currently there's no way to
> avoid getting a largely redundant set of tests on each of those
> sub-platforms.

OK, that makes sense, and I think that's very useful. Reducing our load, especially by making it possible to remove redundant jobs, is a priority for us.


> Guess I should've commented it a bit.

Yeah, it'd be great if you could add a few upon landing.
 
> > I have a couple of comments about the patch itself, but I just want to make
> > sure I understand this correctly before giving it a full review. Sorry for
> > the delay, and thank you for spending time on this!
> 
> Like I said, I'm not even convinced this is that great of an idea,

I think it's a good idea! In some future world we'll have better ways to do this stuff, but this seems like a good approach for now.

 but we
> And if
> you do think this is worth adding, then I wonder if it should also allow
> [-a,b,c] to mean everything *but* jobs matching a, b, or c. That would allow
> eg -u all[-mochi] to get all unit test suites except for mochitests ("I'm
> pushing a patch that I know doesn't affect mochitests, but I don't know what
> all the other tests do so give me all of the rest so I don't need to figure
> it out.")

That'd be great!

> 
> Which points out that I probably ought to make the substring match
> case-insensitive, since the prettyNames might capitalize things randomly.

I think either way is fine here. We don't do any .upper() or .lower() or anything like that on these names.


The patch looks fine, but can you please add a few tests for this to make sure it doesn't break in the future?

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 672656 [details] [diff] [review]
Allow filtering tests by prettyName substring tests

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

Steve pointed out on IRC that there is a test, it's just a bit obscured by the formatting changes!

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Allow filtering tests by prettyName substring tests

Sorry for the re-review request, but I commented some more and added a couple of simple extra tests -- and ended up pretty much rewriting the patch in the process.

This now supports the negation syntax eg mochitests[-10.6,10.7]. It matches against the full resulting builder names; previously, it was only matching against the pretty versions of the platform name. So all my examples of using all[moch] or whatever turned out not to work. This latest version also fixes a problem where grouped test suite specifications (eg 'all' or 'mochitests') would not apply the filters to the proper tests.

I also stripped out the test reformatting that didn't change functionality, to make it easier to see what's going on there.

tl;dr: it sucked in nonobvious ways, I have better tests now, and I rewrote the code to pass those tests.

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 676863 [details] [diff] [review]
Allow filtering tests by prettyName substring tests

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

I have a quibble about the syntax and some test organization, but the rest looks fine.

::: test/test_try_parser.py
@@ +310,5 @@
> +        tm = 'try: -b do -p win32 -u crashtest[-notfound1,notfound2]'
> +        self.customBuilders = TryParser(tm, VALID_TESTER_NAMES, TESTER_PRETTY_NAMES,None, UNITTEST_SUITES)
> +        builders = ['Rev3 WINNT 5.1 try opt test crashtest',
> +                    'Rev3 WINNT 6.1 try opt test crashtest',
> +                    'Rev3 WINNT 6.1 try debug test crashtest']

Hm. It's a bit unexpected to me that you end up with all crashtests when you have an inclusion that happens not match anything. I would expect a syntax like this to end up running no crashtests. Arguably, it's better than running way more tests than needed. What do you think?

Actually, after reading the code it seems like those are both exclusions...if that's the case I think we should move the "-" to be before the testname, eg "-crashtest[notfound1,notfound2]". [-notfound1,notfound] reads to me that you don't want "notfound1" tests, but *do* want "notfound2" tests!

@@ +322,5 @@
> +        self.customBuilders = TryParser(tm, VALID_TESTER_NAMES, TESTER_PRETTY_NAMES,None, UNITTEST_SUITES)
> +        builders = ['Rev3 WINNT 5.1 try opt test reftest',
> +                    'Rev3 WINNT 5.1 try opt test crashtest',
> +                    'Rev3 WINNT 6.1 try opt test crashtest',
> +                    'Rev3 WINNT 6.1 try debug test crashtest']

Please separate these into separate tests. It's harder to develop when you end up skipping test cases because they're all run sequentially in one test rather run separately. Eg, the first one fails and you don't know which others will, which can end up with wasted time on a solution that only works for the first test.

Use a helper method if you want to factor out the common bits, like the TryParser() call.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Allow filtering tests by prettyName substring tests

Ok, I switched to what you expected in the first place, which is for the '-' to bind only to its immediate substring. It simplified the code, too.

I seem to have ended up with two separate comments describing pretty much the same thing.

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 677860 [details] [diff] [review]
Allow filtering tests by prettyName substring tests

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

Sorry for the delay in the review, this looks good though!

Comment 11

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/build/buildbotcustom/rev/e9cd53d34696

Comment 12

[[e-mail] (:bhearsum)]

This went into production ages ago.