Bug 97143 – convert apple mac TestExpectations files to the new syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

convert apple mac TestExpectations files to the new syntax

[Dirk Pranke]

Created attachment 164797 [details]
Patch

[Dirk Pranke]

Committed r129061: <http://trac.webkit.org/changeset/129061>

[Jessie Berlin]

Comment on attachment 164797 [details]
Patch

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

> LayoutTests/platform/mac/TestExpectations:175
> +webkit.org/b/69210 fast/encoding/utf-16-big-endian.html [ Failure Skip ]

http://trac.webkit.org/wiki/TestExpectations says that "WontFix and Skip must be the only expectation and cannot be specified alongside Crash or anything else; since the tests will be skipped, the other expectations will likely become stale."

Is that not actually true? Why specify [ Failure Skip ] here?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 164797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164797&action=review
> 
> > LayoutTests/platform/mac/TestExpectations:175
> > +webkit.org/b/69210 fast/encoding/utf-16-big-endian.html [ Failure Skip ]
> 
> http://trac.webkit.org/wiki/TestExpectations says that "WontFix and Skip must be the only expectation and cannot be specified alongside Crash or anything else; since the tests will be skipped, the other expectations will likely become stale."
> 
> Is that not actually true? Why specify [ Failure Skip ] here?

At the moment, that's not actually true (and a bug). Making them be the only expectation was a relatively late design change that I forgot to implement before the conversion :(. I'll be fixing that today.

[Dirk Pranke]

Note that Skip (and WontFix) should be overriding the other expectations, so this bug shouldn't have any effect at runtime, just be confusing and misleading.

[Alexey Proskuryakov]

> +webkit.org/b/69210 fast/encoding/utf-16-big-endian.html [ Failure Skip ]

I don't see how this bug is related (and there is something fishy going on, as the test passes for me in WK1 locally, but not in WK2).

Can one have TestExpectations entries without bugs? I don't agree that every expectation needs a bug, a short inline comment can be perfectly enough for many,

[Ojan Vafai]

(In reply to comment #6)
> > +webkit.org/b/69210 fast/encoding/utf-16-big-endian.html [ Failure Skip ]
> 
> I don't see how this bug is related (and there is something fishy going on, as the test passes for me in WK1 locally, but not in WK2).

Looks like whoever added that line put the wrong bug number maybe?

> Can one have TestExpectations entries without bugs? I don't agree that every expectation needs a bug, a short inline comment can be perfectly enough for many,

Currently it's a linter error if you don't have a bug or Bug(username) for each line. We've found that, in practice it leads to much better thoroughness when people add lines. We did discuss it in one of the previous webkit-dev threads and the only voice that chimed in was Maciej saying that he liked the idea of keeping bug entries required. In either case, none of that is related to this patch. Feel free to bring it up on webkit-dev if you feel strongly about it.

[Dirk Pranke]

(In reply to comment #6)
> Can one have TestExpectations entries without bugs? I don't agree that every expectation needs a bug, a short inline comment can be perfectly enough for many,

Yes, bugs are not required, but they are encouraged, especially for anything that isn't WontFix. I agree that comments are enough for some situations.

[Dirk Pranke]

(In reply to comment #7)
> Currently it's a linter error if you don't have a bug or Bug(username) for each line.

Technically we don't complain if the bug is marked WontFix, but do otherwise.

[Alexey Proskuryakov]

What's a "linter error"? Will my world fall apart if I add a test expectation without a bug link?

[Ryosuke Niwa]

(In reply to comment #10)
> What's a "linter error"? Will my world fall apart if I add a test expectation without a bug link?

It doesn't. It'll just turn a bot on build.chromium.org red so it's likely that someone will come in and file a bug for it. I support making it optional.

[Dirk Pranke]

(In reply to comment #10)
> What's a "linter error"? Will my world fall apart if I add a test expectation without a bug link?

rwt --lint-test-files and check-webkit-style will issue a warning. The chromium.org canary bots also have a 'webkit_lint' build step that runs rwt --lint-test-files , so that step will fail as well (only if the line is in the chromium files, though). 

Now that everyone is using the TestExpectations files, we should probably add that step to the bots.

[Simon Fraser (smfr)]

(In reply to comment #11)
> (In reply to comment #10)
> > What's a "linter error"? Will my world fall apart if I add a test expectation without a bug link?
> 
> It doesn't. It'll just turn a bot on build.chromium.org red so it's likely that someone will come in and file a bug for it. I support making it optional.

What's the point of just turning a Chromium bot red? Non-Google people will never see them.

[Dirk Pranke]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > What's a "linter error"? Will my world fall apart if I add a test expectation without a bug link?
> > 
> > It doesn't. It'll just turn a bot on build.chromium.org red so it's likely that someone will come in and file a bug for it. I support making it optional.
> 
> What's the point of just turning a Chromium bot red? Non-Google people will never see them.

See comment #12: the step checks the files that the bot actually cares about, so if someone screws up platform/mac/TestExpectations, the chromium bot doesn't turn red. I would imagine that if we add this step to other bots they would work the same way.

There's not much of a performance hit either way, so we could make every bot check every port but I'm not sure that's justified.

[Ryosuke Niwa]

(In reply to comment #14)
> See comment #12: the step checks the files that the bot actually cares about, so if someone screws up platform/mac/TestExpectations, the chromium bot doesn't turn red. I would imagine that if we add this step to other bots they would work the same way.
> 
> There's not much of a performance hit either way, so we could make every bot check every port but I'm not sure that's justified.

Really? I thought we decided to lint all platforms' TestExpectations?

[Dirk Pranke]

(In reply to comment #15)
> (In reply to comment #14)
> > See comment #12: the step checks the files that the bot actually cares about, so if someone screws up platform/mac/TestExpectations, the chromium bot doesn't turn red. I would imagine that if we add this step to other bots they would work the same way.
> > 
> > There's not much of a performance hit either way, so we could make every bot check every port but I'm not sure that's justified.
> 
> Really? I thought we decided to lint all platforms' TestExpectations?

Nope. If you run rwt --lint-test-files you'll see a whole bunch of errors from the other ports about duplicates between the TestExpectations and Skipped files, so this step would always be red on our machine.

Once we remove the Skipped files and people are more consistently dealing with just a single set of sources, it might make more sense to lint all of them, but even then I'm not convinced that another ports' failures should turn a port's bots red.