615776 - Clear up exception on expandTestSuites that is causing try_parser to send default set

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P1)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

Today there was a push to try that looked like:

"desc": "Bug 615146 - Ensure outer SVG elements get an 
                 nsSVGOuterSVGFrame even they fail conditional processing 
                 attributes\ntry: -b o -p linux64 -m none -u reftest -t none"

in the json pushlog info for http://hg.mozilla.org/try/rev/41ff2ed05ffd and instead of only triggering the linux64 build, it triggered all builds. 

I tested the regex matching and splitting with a '\ntry: sample' and was unable to reproduce so look into how this kind of character comes back from the json and find a way to test for this case.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

also noticed that there were preceding pushes that used the (now deprecated) -m none in their pushlog comments and those others got just the selected platform builds they requested (linux in one case, win32 in another) they did not have multiline comments that split right on the try: which is why it's very likely something caused by the '\n'

Comment 2

[Mike Taylor [:bear]]

I would like to suggest that we do a more aggressive filtering to prevent even a chance of user-text from getting into one of our command lines by sending the string you process thru a whitelist of acceptable characters.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

this is the third time someone asking for win32 only has gotten all builds and this time it's not on a newline:

http://hg.mozilla.org/try/rev/0e8764656628

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

My previous statement was incorrect, this was the second time with the first being revision b734477f91a8

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to comment #4)
> My previous statement was incorrect, this was the second time with the first
> being revision b734477f91a8

And for that revision it turns out there was no -b specified, so that needs to be examined.  If there's no -b specified the user should get opt/debug of the platforms requested.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

I've added the following test suites and they all pass:

    def test_HiddenCharactersAndOldSyntax(self):
        tm = 'attributes\ntry: -b o -p linux64 -m none -u reftest -t none'
        self.customBuilders = TryParser(tm, VALID_BUILDER_NAMES, BUILDER_PRETTY_NAMES, None, UNITTEST_SUITES)
        builders = ['Linux x86-64 tryserver build']
        self.assertEqual(sorted(self.customBuilders),sorted(builders))

    def test_NoBuildTypeSelected(self):
        tm = 'try: -m none -u crashtest -p win32' 
        # should get both build types for the selected platform
        self.customBuilders = TryParser(tm, VALID_BUILDER_NAMES, BUILDER_PRETTY_NAMES, None, UNITTEST_SUITES)
        builders = ['WINNT 5.2 tryserver build', 'WINNT 5.2 tryserver leak test build']
        self.assertEqual(sorted(self.customBuilders),sorted(builders))

        # should get debug win32 in the builder_master test builders
        self.customBuilders = TryParser(tm, VALID_BUILDER_NAMES+VALID_UPN, {}, UNITTEST_PRETTY_NAMES, UNITTEST_SUITES)
        builders = ['WINNT 5.2 tryserver debug test crashtest']
        self.assertEqual(sorted(self.customBuilders),sorted(builders))

        # should get both build types in test_builders
        self.customBuilders = TryParser(tm, VALID_TESTER_NAMES, TESTER_PRETTY_NAMES, None, UNITTEST_SUITES)
        builders = ['Rev3 WINNT 6.1 tryserver opt test crashtest', 'Rev3 WINNT 6.1 tryserver debug test crashtest']
        self.assertEqual(sorted(self.customBuilders),sorted(builders))


Not sure how else to debug why occasional failures in try_parser occur.  Possibly something on the twisted end of things?  If it can't locate the json to get the desc, the pusher will end up with the default set.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

The fix for this is included in https://bugzilla.mozilla.org/attachment.cgi?id=494600 which will land tomorrow morning so long as the release schedule allows.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

No more exceptions in the scheduler since the fix was landed.