Bug 44648 – new-run-webkit-tests: add rebaselining tests for test_expectations

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

more unit tests ...

[Dirk Pranke]

Created attachment 65500 [details]
Patch

[Ojan Vafai]

Comment on attachment 65500 [details]
Patch

> +    def test_remove_expand(self):
> +        # This test is somewhat counterintuitive - lines with no platform
> +        # specified get expanded to every platform.
> +        self.assertRemove('',
> +            'BUGX REBASELINE : failures/expected/text.html = TEXT\n',
> +            'BUGX REBASELINE MAC WIN : failures/expected/text.html = TEXT\n')

This looks wrong. Mind adding a FIXME and filing a bug CCed to victorw? It's not a big deal, but it does bloat test_expectations.

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 65500 [details])
> > +    def test_remove_expand(self):
> > +        # This test is somewhat counterintuitive - lines with no platform
> > +        # specified get expanded to every platform.
> > +        self.assertRemove('',
> > +            'BUGX REBASELINE : failures/expected/text.html = TEXT\n',
> > +            'BUGX REBASELINE MAC WIN : failures/expected/text.html = TEXT\n')
> 
> This looks wrong. Mind adding a FIXME and filing a bug CCed to victorw? It's not a big deal, but it does bloat test_expectations.

If it is wrong, I'd just as soon fix the bug here. 

Since we reasonably routinely rebaseline across all platforms, I'm assuming that the rebaseline_chromium_webkit_tests code is smart enough to handle this case somehow. Either that, or it traps this case before every calling this function, but that seems unlikely. 

Victor?

[Victor Wang]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 65500 [details] [details])
> > > +    def test_remove_expand(self):
> > > +        # This test is somewhat counterintuitive - lines with no platform
> > > +        # specified get expanded to every platform.
> > > +        self.assertRemove('',
> > > +            'BUGX REBASELINE : failures/expected/text.html = TEXT\n',
> > > +            'BUGX REBASELINE MAC WIN : failures/expected/text.html = TEXT\n')
> > 
> > This looks wrong. Mind adding a FIXME and filing a bug CCed to victorw? It's not a big deal, but it does bloat test_expectations.
> 
> If it is wrong, I'd just as soon fix the bug here. 
> 
> Since we reasonably routinely rebaseline across all platforms, I'm assuming that the rebaseline_chromium_webkit_tests code is smart enough to handle this case somehow. Either that, or it traps this case before every calling this function, but that seems unlikely. 
> 
> Victor?

We should return the original string if the platform param is empty.

The purpose is if there is a test expectation like this:
BUGX REBASELINE : failures/expected/text.html = TEXT\n

and we did rebaseline one platform: LINUX, the line should change to
BUGX REBASELINE WIN MAC: failures/expected/text.html = TEXT\n

The rebaseline_chromium_webkit_tests should never pass empty platform as param, but to make the code more robust, I think we could just return the original line in remove_platform_from_expectations if platform is empty.

By the way, thanks for adding unittests.

[Victor Wang]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 65500 [details] [details] [details])
> > > > +    def test_remove_expand(self):
> > > > +        # This test is somewhat counterintuitive - lines with no platform
> > > > +        # specified get expanded to every platform.
> > > > +        self.assertRemove('',
> > > > +            'BUGX REBASELINE : failures/expected/text.html = TEXT\n',
> > > > +            'BUGX REBASELINE MAC WIN : failures/expected/text.html = TEXT\n')
> > > 
> > > This looks wrong. Mind adding a FIXME and filing a bug CCed to victorw? It's not a big deal, but it does bloat test_expectations.
> > 
> > If it is wrong, I'd just as soon fix the bug here. 
> > 
> > Since we reasonably routinely rebaseline across all platforms, I'm assuming that the rebaseline_chromium_webkit_tests code is smart enough to handle this case somehow. Either that, or it traps this case before every calling this function, but that seems unlikely. 
> > 
> > Victor?
> 
> We should return the original string if the platform param is empty.
> 
> The purpose is if there is a test expectation like this:
> BUGX REBASELINE : failures/expected/text.html = TEXT\n
> 
> and we did rebaseline one platform: LINUX, the line should change to
> BUGX REBASELINE WIN MAC: failures/expected/text.html = TEXT\n
> 
> The rebaseline_chromium_webkit_tests should never pass empty platform as param, but to make the code more robust, I think we could just return the original line in remove_platform_from_expectations if platform is empty.
> 
> By the way, thanks for adding unittests.

so the test should looks like this once you change remove_platform_from_expectations to return the original line when platform is empty:
    self.assertRemove('',
            'BUGX REBASELINE : failures/expected/text.html = TEXT\n',
            'BUGX REBASELINE : failures/expected/text.html = TEXT\n')

[Dirk Pranke]

Okay, this makes sense. I'll revise the code and add a couple more tests to ensure we cover these two cases.

[Dirk Pranke]

Created attachment 65511 [details]
Patch

[Dirk Pranke]

(In reply to comment #7)
> Created an attachment (id=65511) [details]
> Patch

Okay, I added one more test to cover both cases here; this feels much more correct. Thanks!

[Victor Wang]

(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=65511) [details] [details]
> > Patch
> 
> Okay, I added one more test to cover both cases here; this feels much more correct. Thanks!

how about return f_orig at the beginning of function remove_platform_from_expectations? this way we don't parse each test expectation line. 

You could also return NO_CHANGE in _get_platform_update_action like this (passing platform as empty should be considered as invalid):
        if not test or test not in tests or not platform:
            return NO_CHANGE

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Created an attachment (id=65511) [details] [details] [details]
> > > Patch
> > 
> > Okay, I added one more test to cover both cases here; this feels much more correct. Thanks!
> 
> how about return f_orig at the beginning of function remove_platform_from_expectations? this way we don't parse each test expectation line. 
> 
> You could also return NO_CHANGE in _get_platform_update_action like this (passing platform as empty should be considered as invalid):
>         if not test or test not in tests or not platform:
>             return NO_CHANGE

Wait, are you saying that we shouldn't modify the lines if platform is empty *regardless* of whether or not a platform was specified in the options?

[Dirk Pranke]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > Created an attachment (id=65511) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > Okay, I added one more test to cover both cases here; this feels much more correct. Thanks!
> > 
> > how about return f_orig at the beginning of function remove_platform_from_expectations? this way we don't parse each test expectation line. 
> > 
> > You could also return NO_CHANGE in _get_platform_update_action like this (passing platform as empty should be considered as invalid):
> >         if not test or test not in tests or not platform:
> >             return NO_CHANGE
> 
> Wait, are you saying that we shouldn't modify the lines if platform is empty *regardless* of whether or not a platform was specified in the options?

If that's the case, then that basically means that platform should never be empty. If that's supposed to be true, I'd prefer to change the code to assert that and check for it, rather than having special cases here to handle usages that shouldn't happen.

[Victor Wang]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (In reply to comment #7)
> > > > > Created an attachment (id=65511) [details] [details] [details] [details] [details]
> > > > > Patch
> > > > 
> > > > Okay, I added one more test to cover both cases here; this feels much more correct. Thanks!
> > > 
> > > how about return f_orig at the beginning of function remove_platform_from_expectations? this way we don't parse each test expectation line. 
> > > 
> > > You could also return NO_CHANGE in _get_platform_update_action like this (passing platform as empty should be considered as invalid):
> > >         if not test or test not in tests or not platform:
> > >             return NO_CHANGE
> > 
> > Wait, are you saying that we shouldn't modify the lines if platform is empty *regardless* of whether or not a platform was specified in the options?
> 
> If that's the case, then that basically means that platform should never be empty. If that's supposed to be true, I'd prefer to change the code to assert that and check for it, rather than having special cases here to handle usages that shouldn't happen.

assert if platform is empty sounds good to me. The platform param should never be empty and chromium rebaseline tool should always set it.

[Dirk Pranke]

Created attachment 65514 [details]
Patch

[Dirk Pranke]

(In reply to comment #12)
> > > Wait, are you saying that we shouldn't modify the lines if platform is empty *regardless* of whether or not a platform was specified in the options?
> > 
> > If that's the case, then that basically means that platform should never be empty. If that's supposed to be true, I'd prefer to change the code to assert that and check for it, rather than having special cases here to handle usages that shouldn't happen.
> 
> assert if platform is empty sounds good to me. The platform param should never be empty and chromium rebaseline tool should always set it.

Okay, patch uploaded that checks to make sure that that is true :)

[Victor Wang]

(In reply to comment #14)
> (In reply to comment #12)
> > > > Wait, are you saying that we shouldn't modify the lines if platform is empty *regardless* of whether or not a platform was specified in the options?
> > > 
> > > If that's the case, then that basically means that platform should never be empty. If that's supposed to be true, I'd prefer to change the code to assert that and check for it, rather than having special cases here to handle usages that shouldn't happen.
> > 
> > assert if platform is empty sounds good to me. The platform param should never be empty and chromium rebaseline tool should always set it.
> 
> Okay, patch uploaded that checks to make sure that that is true :)

LGTM (I am not a reviewer). Thanks again for adding unittests for this.

[Dirk Pranke]

Committed r66125: <http://trac.webkit.org/changeset/66125>