Bug 86690 – Rename test_expectations.txt to TestExpectations

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

We should rename test_expectations.txt to TestExpectations to match WebKit's naming convention.

[Adam Barth]

Other naming ideas:

* Expectations
* ExpectedResults
* CurrentResults

I'm tempted by the last one because it avoids the confusing between this file and the -expected.txt files.

[Ryosuke Niwa]

How about ExpectedOutcome to avoid confusion with -expected.* files?

[Dirk Pranke]

Unless someone comes up with something massively better than what we have now this seems like semi-pointless bikeshedding.

[Adam Barth]

@Dirk, you're no fun!  :)

[Ryosuke Niwa]

(In reply to comment #3)
> Unless someone comes up with something massively better than what we have now this seems like semi-pointless bikeshedding.

I think it'll be nice to come up with a name other than TestExpectations because that directly conflict with -expected.* files for historical reasons.

[Dirk Pranke]

(In reply to comment #4)
> @Dirk, you're no fun!  :)

Sorry :).

[Maciej Stachowiak]

(In reply to comment #1)
> Other naming ideas:
> 
> * Expectations
> * ExpectedResults
> * CurrentResults
> 
> I'm tempted by the last one because it avoids the confusing between this file and the -expected.txt files.

I like CurrentResults for the same reason as Adam.

[Maciej Stachowiak]

(In reply to comment #3)
> Unless someone comes up with something massively better than what we have now this seems like semi-pointless bikeshedding.

As a counterpoint: WebKit has a long tradition of renaming and moving things for clarity and consistency. We try not to rename things willy-nilly but nearly every file in the project has been renamed at least once and has moved places in the directory structure at least twice. If we find a name that is genuinely better, we should try not to be held back by inertia alone. That is the WebKit Way as I understand it.

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #1)
> > Other naming ideas:
> > 
> > * Expectations
> > * ExpectedResults
> > * CurrentResults
> > 
> > I'm tempted by the last one because it avoids the confusing between this file and the -expected.txt files.
> 
> I like CurrentResults for the same reason as Adam.

Is the confusion you're seeking to avoid the use of "expect" in both names? Or is it that "test expectations" sounds too much like "text expectation"? Can someone use these names in an example sentence or two to better illustrate the confusion? 

For example, I often ask someone "what's in the test expectations" or say "I will add that to the expectations file"; it seems like the meaning of both of those is reasonably clear, but I could see how the first one could be misheard.

Maybe something like "suppressions" is better, since we often talk about suppressing an error?

On a different note, I would like to note that there is a clear connection between the name of the file and the name of the classes that process the file in the code today (e.g., TestExpectaions{Parser,Serializer,Model}). If we change the one, we should probably change the others as well. To that end, I am somewhat leery of "result" insofar as that is already badly overloaded in the NRWT code (e.g., TestResult, ResultSummary).

(In reply to comment #8)
> (In reply to comment #3)
> > Unless someone comes up with something massively better than what we have now this seems like semi-pointless bikeshedding.
> 
> As a counterpoint: WebKit has a long tradition of renaming and moving things for clarity and consistency. We try not to rename things willy-nilly but nearly every file in the project has been renamed at least once and has moved places in the directory structure at least twice. If we find a name that is genuinely better, we should try not to be held back by inertia alone. That is the WebKit Way as I understand it.

Sure, I agree with that. I think it's all in how one defines "genuinely" (or "massively") better (and willy-nilly, for that matter).

[Ryosuke Niwa]

(In reply to comment #9)
> Is the confusion you're seeking to avoid the use of "expect" in both names? Or is it that "test expectations" sounds too much like "text expectation"? Can someone use these names in an example sentence or two to better illustrate the confusion? 

"Test expectations of my-test.html have changed since r12345, and we need to rebaseline them."  Here, the speaker is referring to expected results as test expectations.

> For example, I often ask someone "what's in the test expectations" or say "I will add that to the expectations file"; it seems like the meaning of both of those is reasonably clear, but I could see how the first one could be misheard.

The problem is that people have used test expectations to mean expected results before nrwt existed.

> Maybe something like "suppressions" is better, since we often talk about suppressing an error?

TestSuppressions? or FailureSuppressions? I like that!

> On a different note, I would like to note that there is a clear connection between the name of the file and the name of the classes that process the file in the code today (e.g., TestExpectaions{Parser,Serializer,Model}). If we change the one, we should probably change the others as well. To that end, I am somewhat leery of "result" insofar as that is already badly overloaded in the NRWT code (e.g., TestResult, ResultSummary).

Yeah, I dislike CurrentResults for that reason. "current results" sound as if we're talking about the current result (i.e. -expected.* files in the sense of results we currently expect) of tests.

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (In reply to comment #3)
> > Unless someone comes up with something massively better than what we have now this seems like semi-pointless bikeshedding.
> 
> As a counterpoint: WebKit has a long tradition of renaming and moving things for clarity and consistency. We try not to rename things willy-nilly but nearly every file in the project has been renamed at least once and has moved places in the directory structure at least twice. If we find a name that is genuinely better, we should try not to be held back by inertia alone. That is the WebKit Way as I understand it.

One thing to mention is that renaming this file will surely break tools -- some even in the WebKit base. So far, I can't see any reason beyond minor esthetics to rename this. Wouldn't we be better off spending our cycles making the file smaller? :)

[Ryosuke Niwa]

(In reply to comment #11)
> One thing to mention is that renaming this file will surely break tools -- some even in the WebKit base. So far, I can't see any reason beyond minor esthetics to rename this. Wouldn't we be better off spending our cycles making the file smaller? :)

We should update all those tools. My plan is to replace all instances of the word "text_expectations.txt" with a new file name.

Enforcing consistency is a good thing because it reduces the mental power I need to process them.

[Dimitri Glazkov (Google)]

(In reply to comment #12)

> Enforcing consistency is a good thing because it reduces the mental power I need to process them.

Please also consider the collective mental power that will go into learning this change.

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #12)
> > Enforcing consistency is a good thing because it reduces the mental power I need to process them.
> 
> Please also consider the collective mental power that will go into learning this change.

Historically, project's assumption has been that this cost is lower than the collective mental power we'll require future contributors to pay. If that were not the case, we wouldn't be changing directory structure, moving source files around, etc... for the fear of one-time learning cost.

[Ryosuke Niwa]

Created attachment 145253 [details]
Make tools aware of TestExpectations in addition to test_expectations.txt

[WebKit Review Bot]

Attachment 145253 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py:77:  too many blank lines (2)  [pep8/E303] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

I'll rename files using "svn mv" once this patch is landed. I can't upload a patch for the actual rename, however, since it will conflict whenever someone modifies test_expectations.txt.

[Ryosuke Niwa]

Created attachment 145254 [details]
Make tools aware of TestExpectations in addition to test_expectations.txt

[Ojan Vafai]

I don't feel strongly about doing or not doing the rename, but if we do the rename, my vote goes for Suppressions. It looks analogous to Skipped and avoids using either the term "results" or "expectations" which are both heavily overloaded and have confused people time and time again.

[Ryosuke Niwa]

(In reply to comment #19)
> I don't feel strongly about doing or not doing the rename, but if we do the rename, my vote goes for Suppressions. It looks analogous to Skipped and avoids using either the term "results" or "expectations" which are both heavily overloaded and have confused people time and time again.

Would FailureSuppressions work for you?

[Ojan Vafai]

(In reply to comment #20)
> (In reply to comment #19)
> > I don't feel strongly about doing or not doing the rename, but if we do the rename, my vote goes for Suppressions. It looks analogous to Skipped and avoids using either the term "results" or "expectations" which are both heavily overloaded and have confused people time and time again.
> 
> Would FailureSuppressions work for you?

FailureSuppressions as opposed to what other kind of suppressions? But, meh, I don't really care if this bikeshed is red or pink. :)

[Ryosuke Niwa]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > I don't feel strongly about doing or not doing the rename, but if we do the rename, my vote goes for Suppressions. It looks analogous to Skipped and avoids using either the term "results" or "expectations" which are both heavily overloaded and have confused people time and time again.
> > 
> > Would FailureSuppressions work for you?
> 
> FailureSuppressions as opposed to what other kind of suppressions? But, meh, I don't really care if this bikeshed is red or pink. :)

That's a good point. I wanted to avoid the auto-completion collision with "Skipped" but I guess it's not a big deal given we've been talking about merging Skipped into TestExpectations.

[Dirk Pranke]

Suppressions works for me as well; if we are going to rename the file I prefer that to TestExpectations or FailureExpectations, but I don't much care about the name one way or another. 

Ojan, were you reviewing this otherwise, or did you want me to do it?

[Dimitri Glazkov (Google)]

After 5 different naming proposals have been suggested, the name must be immediately chosen as "Purple".

[Ojan Vafai]

(In reply to comment #23)
> Ojan, were you reviewing this otherwise, or did you want me to do it?

Go for it. Sounds like we have consensus on Suppressions?

(In reply to comment #24)
> After 5 different naming proposals have been suggested, the name must be immediately chosen as "Purple".

I prefer "iframe". iframe needs to be a color, preferably one connoting doom.

[Maciej Stachowiak]

I think it would be better to name the file after the information it contains than what effect it has. So I like the original suggestion of TestExpectations (or the alternate suggestion of CurrentResults) better than Suppressions (or similar). Yes, I realize Skipped does not meet this standard, I think that is one of its disadvantages.

[Dirk Pranke]

(In reply to comment #26)
> I think it would be better to name the file after the information it contains than what effect it has. So I like the original suggestion of TestExpectations (or the alternate suggestion of CurrentResults) better than Suppressions (or similar). Yes, I realize Skipped does not meet this standard, I think that is one of its disadvantages.

I could argue that it contains suppressions, not expectations, given that it doesn't contain a list of "PASS" for all of the other files that we expect to just pass. But that, like most things in this bug, is quibbling :).

[Ojan Vafai]

Just to be clear, don't hold anything up on my opinion. I have a vague preference for the color of this bikeshed, but don't really care.

[Ryosuke Niwa]

Given that Maciej appears to have a strong preference and everyone else doesn't care as much, I'm inclined to use TestExpectations as I had initially planned.

[Dirk Pranke]

Comment on attachment 145254 [details]
Make tools aware of TestExpectations in addition to test_expectations.txt

View in context: https://bugs.webkit.org/attachment.cgi?id=145254&action=review

The patch is generally fine and may be r+'able as-is (with a couple of nits about reducing repetitive mentions of the path) but I'd like to understand the answers to my questions before r+'ing it

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:-677
> -

Ick. I'm glad this wasn't being used :).

> Tools/Scripts/webkitpy/layout_tests/port/base.py:688
> +            port_name = 'chromium'

Why are you pulling the logic out of webkit.py and chromium.py and pulling it into this routine? If it's to merge the logic of Port and WebKitPort, I'd prefer that to be addressed in a different patch dedicated to that just to avoid confusion. 

Do you have a preference for avoiding overridden functions or something as well that's leading to this change?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:694
> +        return self._filesystem.join(baseline_path, 'TestExpectations')

Why are you doing this rather than just renaming all of the files?

> Tools/Scripts/webkitpy/layout_tests/port/test.py:251
> +        filesystem.write_text_file(LAYOUT_TEST_DIR + '/platform/test/TestExpectations', """

Can you pull '/platform/test/TestExpectations' into a constant somewhere so that it is shared between this and line 343, below?

> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:97
> +        self.assertEqual(port.path_to_test_expectations_file(), '/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations')

We should pull '/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations' into a constant as well so it can be re-used across multiple tests.

> Tools/Scripts/webkitpy/style/checker.py:527
> +        elif basename == 'test_expectations.txt' or basename == 'TestExpectations':

Here's another place where I don't know that we need to support both syntaxes simultaneously.

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:64
> +        self._output_regex = re.compile('.*(TestExpectations|test_expectations.txt):(?P<line>\d+)\s*(?P<message>.+)')

ditto.

> Tools/Scripts/webkitpy/tool/steps/commit.py:59
> +            if filename.endswith('test_expectations.txt') or filename.endswith('TestExpectations'):

ditto.

> Tools/TestResultServer/static-dashboards/dashboard_base.js:546
> +var LEGACY_CHROMIUM_EXPECTATIONS_URL = 'http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium/test_expectations.txt';

Do we have to support both URLs for a reason other than the reason you want to support both filenames generally?

[Ryosuke Niwa]

Comment on attachment 145254 [details]
Make tools aware of TestExpectations in addition to test_expectations.txt

View in context: https://bugs.webkit.org/attachment.cgi?id=145254&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:688
>> +            port_name = 'chromium'
> 
> Why are you pulling the logic out of webkit.py and chromium.py and pulling it into this routine? If it's to merge the logic of Port and WebKitPort, I'd prefer that to be addressed in a different patch dedicated to that just to avoid confusion. 
> 
> Do you have a preference for avoiding overridden functions or something as well that's leading to this change?

Okay. The reason I merged these two functions is so that I don't have to duplicate the if condition below in both WebKit and Chromium port code.
But I'm fine with making duplicated changes in both classes.

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:694
>> +        return self._filesystem.join(baseline_path, 'TestExpectations')
> 
> Why are you doing this rather than just renaming all of the files?

Okay, will do that.

>> Tools/Scripts/webkitpy/layout_tests/port/test.py:251
>> +        filesystem.write_text_file(LAYOUT_TEST_DIR + '/platform/test/TestExpectations', """
> 
> Can you pull '/platform/test/TestExpectations' into a constant somewhere so that it is shared between this and line 343, below?

Sure. Will do that.

>> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:97
>> +        self.assertEqual(port.path_to_test_expectations_file(), '/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations')
> 
> We should pull '/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations' into a constant as well so it can be re-used across multiple tests.

Will do.

>> Tools/Scripts/webkitpy/style/checker.py:527
>> +        elif basename == 'test_expectations.txt' or basename == 'TestExpectations':
> 
> Here's another place where I don't know that we need to support both syntaxes simultaneously.

Will do.

>> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:64
>> +        self._output_regex = re.compile('.*(TestExpectations|test_expectations.txt):(?P<line>\d+)\s*(?P<message>.+)')
> 
> ditto.

Sure.

>> Tools/Scripts/webkitpy/tool/steps/commit.py:59
>> +            if filename.endswith('test_expectations.txt') or filename.endswith('TestExpectations'):
> 
> ditto.

Will do.

>> Tools/TestResultServer/static-dashboards/dashboard_base.js:546
>> +var LEGACY_CHROMIUM_EXPECTATIONS_URL = 'http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium/test_expectations.txt';
> 
> Do we have to support both URLs for a reason other than the reason you want to support both filenames generally?

Yes, dashboard needs to be updated first.

[Dirk Pranke]

(In reply to comment #31)
> (From update of attachment 145254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145254&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/base.py:688
> >> +            port_name = 'chromium'
> > 
> > Why are you pulling the logic out of webkit.py and chromium.py and pulling it into this routine? If it's to merge the logic of Port and WebKitPort, I'd prefer that to be addressed in a different patch dedicated to that just to avoid confusion. 
> > 
> > Do you have a preference for avoiding overridden functions or something as well that's leading to this change?
> 
> Okay. The reason I merged these two functions is so that I don't have to duplicate the if condition below in both WebKit and Chromium port code.
> But I'm fine with making duplicated changes in both classes.
> 

Yeah, let's just duplicate the changes for now, it's slightly less confusing, to me at least.

> > Do we have to support both URLs for a reason other than the reason you want to support both filenames generally?
> 
> Yes, dashboard needs to be updated first.

As discussed, we could also just break the dashboard for the time it'll take to update the app, but it's up to you.

[Ryosuke Niwa]

Committed r119314: <http://trac.webkit.org/changeset/119314>