Bug 86690 - Rename test_expectations.txt to TestExpectations
Summary: Rename test_expectations.txt to TestExpectations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 88164 89038
  Show dependency treegraph
 
Reported: 2012-05-16 16:50 PDT by Ryosuke Niwa
Modified: 2012-06-13 14:22 PDT (History)
10 users (show)

See Also:


Attachments
Make tools aware of TestExpectations in addition to test_expectations.txt (30.09 KB, patch)
2012-06-01 02:57 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Make tools aware of TestExpectations in addition to test_expectations.txt (30.09 KB, patch)
2012-06-01 03:01 PDT, Ryosuke Niwa
dpranke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-05-16 16:50:04 PDT
We should rename test_expectations.txt to TestExpectations to match WebKit's naming convention.
Comment 1 Adam Barth 2012-05-16 20:25:51 PDT
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.
Comment 2 Ryosuke Niwa 2012-05-16 20:33:53 PDT
How about ExpectedOutcome to avoid confusion with -expected.* files?
Comment 3 Dirk Pranke 2012-05-16 20:49:28 PDT
Unless someone comes up with something massively better than what we have now this seems like semi-pointless bikeshedding.
Comment 4 Adam Barth 2012-05-16 20:52:11 PDT
@Dirk, you're no fun!  :)
Comment 5 Ryosuke Niwa 2012-05-16 20:53:08 PDT
(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.
Comment 6 Dirk Pranke 2012-05-16 21:44:53 PDT
(In reply to comment #4)
> @Dirk, you're no fun!  :)

Sorry :).
Comment 7 Maciej Stachowiak 2012-05-16 22:08:29 PDT
(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.
Comment 8 Maciej Stachowiak 2012-05-16 22:18:54 PDT
(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.
Comment 9 Dirk Pranke 2012-05-16 22:30:46 PDT
(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).
Comment 10 Ryosuke Niwa 2012-05-16 22:39:55 PDT
(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.
Comment 11 Dimitri Glazkov (Google) 2012-05-17 07:26:04 PDT
(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? :)
Comment 12 Ryosuke Niwa 2012-05-17 10:13:15 PDT
(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.
Comment 13 Dimitri Glazkov (Google) 2012-05-17 10:14:44 PDT
(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.
Comment 14 Ryosuke Niwa 2012-05-17 10:19:45 PDT
(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.
Comment 15 Ryosuke Niwa 2012-06-01 02:57:23 PDT
Created attachment 145253 [details]
Make tools aware of TestExpectations in addition to test_expectations.txt
Comment 16 WebKit Review Bot 2012-06-01 02:58:47 PDT
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.
Comment 17 Ryosuke Niwa 2012-06-01 02:59:16 PDT
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.
Comment 18 Ryosuke Niwa 2012-06-01 03:01:53 PDT
Created attachment 145254 [details]
Make tools aware of TestExpectations in addition to test_expectations.txt
Comment 19 Ojan Vafai 2012-06-01 09:29:42 PDT
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.
Comment 20 Ryosuke Niwa 2012-06-01 11:04:29 PDT
(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?
Comment 21 Ojan Vafai 2012-06-01 11:28:19 PDT
(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. :)
Comment 22 Ryosuke Niwa 2012-06-01 12:07:11 PDT
(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.
Comment 23 Dirk Pranke 2012-06-01 12:08:32 PDT
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?
Comment 24 Dimitri Glazkov (Google) 2012-06-01 12:11:22 PDT
After 5 different naming proposals have been suggested, the name must be immediately chosen as "Purple".
Comment 25 Ojan Vafai 2012-06-01 12:20:50 PDT
(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.
Comment 26 Maciej Stachowiak 2012-06-01 13:49:21 PDT
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.
Comment 27 Dirk Pranke 2012-06-01 15:00:54 PDT
(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 :).
Comment 28 Ojan Vafai 2012-06-01 15:07:31 PDT
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.
Comment 29 Ryosuke Niwa 2012-06-01 15:38:30 PDT
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.
Comment 30 Dirk Pranke 2012-06-01 16:40:14 PDT
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?
Comment 31 Ryosuke Niwa 2012-06-01 17:00:43 PDT
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.
Comment 32 Dirk Pranke 2012-06-01 17:03:50 PDT
(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.
Comment 33 Ryosuke Niwa 2012-06-01 23:44:14 PDT
Committed r119314: <http://trac.webkit.org/changeset/119314>