Bug 86796 – Change a bunch of things in the test_expectations.txt format

• Component: Tools / Tests
• Status: RESOLVED DUPLICATE

[Ojan Vafai]

As per the result of https://lists.webkit.org/pipermail/webkit-dev/2012-May/020674.html, we've agree to make the following changes to the test_expectations.txt format.

1. Make SLOW, SKIP and WONTFIX outcomes instead of modifiers.
2. Change bug modifiers to be URLs. The format for person bugs is bug(ojan).
3. The only things that come before the test are bug modifiers and platform/configuration modifiers (e.g. MAC DEBUG).
4. WONTFIX tests are either skipped or we ignore their output as long as they don't crash (slight preference for skipping them, but not enough to argue over it).
5. Use the same character as a separator before and after the test (":").
6. Standardize both Skipped and test_expectations.txt on a common comment delimiter (i.e. "#").

Anyone feel moved to make these changes?

[Dirk Pranke]

(In reply to comment #0)
> As per the result of https://lists.webkit.org/pipermail/webkit-dev/2012-May/020674.html, we've agree to make the following changes to the test_expectations.txt format.

Not to be a huge pain, but I'm not sure that there was "agreement" on this; I'd kinda like to see Maciej and/or Darin weigh in.

For the record, though ...
> 
> 1. Make SLOW, SKIP and WONTFIX outcomes instead of modifiers.

Does SLOW imply PASS? We certainly have slow failures today, and if it doesn't imply pass, but only keeps its current semantics, it seems like it complicates the implementation (SLOW is now "special"). We could also just get rid of SLOW.

> 4. WONTFIX tests are either skipped or we ignore their output as long as they don't crash (slight preference for skipping them, but not enough to argue over it).

We should skip the WONTFIX tests. I don't think we should ignore output if we're going to run tests. 

> 2. Change bug modifiers to be URLs. The format for person bugs is bug(ojan).

I'm fine w/ this.

> 3. The only things that come before the test are bug modifiers and platform/configuration modifiers (e.g. MAC DEBUG).

I'm fine w/ this (although unclear about if this is true depending on how we deal w/ SLOW).

> 5. Use the same character as a separator before and after the test (":").

I'm not sure this is an improvement. If we were going to standardize on the same delimiter I would be open to using one that doesn't require a shift key as per request from rniwa. Possible candidates: "-", ";", ",", "=" (which I would prefer in that order, I think; frankly, I only really like "-" here, and I'm not sure I like any of them more than the existing ":"/"=" combo).

> 6. Standardize both Skipped and test_expectations.txt on a common comment delimiter (i.e. "#").

Fine w/ this one. I have no preference between "#" and "//".
 
> Anyone feel moved to make these changes?

I woud be happy to do some or all of the changes once the dust settles on this debate.

[Ryosuke Niwa]

(In reply to comment #1)
> (In reply to comment #0)
> > As per the result of https://lists.webkit.org/pipermail/webkit-dev/2012-May/020674.html, we've agree to make the following changes to the test_expectations.txt format.
> 
> Not to be a huge pain, but I'm not sure that there was "agreement" on this; I'd kinda like to see Maciej and/or Darin weigh in.

It's my understanding that nobody has objected to these changes so far so we're on rough consensus. We don't necessarily ask everyone to explicitly say they concur since WebKit isn't really a standard body but I think that's okay.

> For the record, though ...
> > 
> > 1. Make SLOW, SKIP and WONTFIX outcomes instead of modifiers.
> 
> Does SLOW imply PASS? We certainly have slow failures today, and if it doesn't imply pass, but only keeps its current semantics, it seems like it complicates the implementation (SLOW is now "special"). We could also just get rid of SLOW.

I think we're not changing the semantics of SLOW. So if we have
wkbug.com/12345 : WIN MAC test.html : SLOW IMAGE TEXT
then that means the test is slow, and may have image or text failures.

> > 4. WONTFIX tests are either skipped or we ignore their output as long as they don't crash (slight preference for skipping them, but not enough to argue over it).
> 
> We should skip the WONTFIX tests. I don't think we should ignore output if we're going to run tests. 

Yes.

> > 5. Use the same character as a separator before and after the test (":").
> 
> I'm not sure this is an improvement. If we were going to standardize on the same delimiter I would be open to using one that doesn't require a shift key as per request from rniwa. Possible candidates: "-", ";", ",", "=" (which I would prefer in that order, I think; frankly, I only really like "-" here, and I'm not sure I like any of them more than the existing ":"/"=" combo).

I would prefer ";" or "," because they are used as delimiters in English sentences and many programming languages.

Or maybe [ ] as in:

webkit.org/12345 WIN MAC DEBUG [ animations/stop-animation-on-suspend.html ] CRASH TEXT PASS

This syntax begs for allowing multiple tests inside [ and ] though:

webkit.org/12345 WIN MAC DEBUG [ animations/my-test.html, animations/another-test.html ] CRASH TEXT PASS

It's probably another controversial change... so I'm not going to formally propose it.

[Ojan Vafai]

(In reply to comment #1)
> (In reply to comment #0)
> > As per the result of https://lists.webkit.org/pipermail/webkit-dev/2012-May/020674.html, we've agree to make the following changes to the test_expectations.txt format.
> 
> Not to be a huge pain, but I'm not sure that there was "agreement" on this; I'd kinda like to see Maciej and/or Darin weigh in.

Meh. I think if anyone were to object, they would have done so by now.

> For the record, though ...
> > 
> > 1. Make SLOW, SKIP and WONTFIX outcomes instead of modifiers.
> 
> Does SLOW imply PASS? We certainly have slow failures today, and if it doesn't imply pass, but only keeps its current semantics, it seems like it complicates the implementation (SLOW is now "special"). We could also just get rid of SLOW.

Yes. Slow needs to imply pass, unless you explicitly list another expected outcome as well. So, SLOW=>PASS, "SLOW TEXT"=>TEXT, "SLOW TEXT PASS"=>"TEXT PASS".

Specifically "SLOW TEXT" does not imply "SLOW PASS TEXT".

Personally, I prefer killing SLOW, but this has been controversial in the past and we've already had enough bikeshedding. :)

> > 4. WONTFIX tests are either skipped or we ignore their output as long as they don't crash (slight preference for skipping them, but not enough to argue over it).
> 
> We should skip the WONTFIX tests. I don't think we should ignore output if we're going to run tests. 

Works for me.

> > 5. Use the same character as a separator before and after the test (":").
> 
> I'm not sure this is an improvement. If we were going to standardize on the same delimiter I would be open to using one that doesn't require a shift key as per request from rniwa. Possible candidates: "-", ";", ",", "=" (which I would prefer in that order, I think; frankly, I only really like "-" here, and I'm not sure I like any of them more than the existing ":"/"=" combo).

I don't really care what we use. Semi-colon seems fine to me. I've just seen it be a source of confusion that there are two different delimiters. 

> > Anyone feel moved to make these changes?
> 
> I woud be happy to do some or all of the changes once the dust settles on this debate.

Great. Thanks! I'm happy to do reviews for this stuff.

[Ryosuke Niwa]

Here are some variants (with blackslash included):

webkit.org/12345 WIN MAC DEBUG \ animations/stop-animation-on-suspend.html \ CRASH TEXT PASS
webkit.org/12345 WIN MAC DEBUG [ animations/stop-animation-on-suspend.html ] CRASH TEXT PASS
webkit.org/12345 WIN MAC DEBUG : animations/stop-animation-on-suspend.html : CRASH TEXT PASS
webkit.org/12345 WIN MAC DEBUG ; animations/stop-animation-on-suspend.html ; CRASH TEXT PASS
webkit.org/12345 WIN MAC DEBUG = animations/stop-animation-on-suspend.html = CRASH TEXT PASS
webkit.org/12345 WIN MAC DEBUG - animations/stop-animation-on-suspend.html - CRASH TEXT PASS
webkit.org/12345 WIN MAC DEBUG , animations/stop-animation-on-suspend.html , CRASH TEXT PASS
webkit.org/12345 WIN MAC DEBUG . animations/stop-animation-on-suspend.html . CRASH TEXT PASS

I kind of dig backslash and square bracket versions. Those two separates tokens visually very well.

[Ryosuke Niwa]

By the way, if we already have 2 delimiters, then why not 3 as in?

webkit.org/12345 \ WIN MAC DEBUG \ animations/stop-animation-on-suspend.html \ CRASH TEXT PASS

Since there are logically 4 separate sections in this line.

[Ojan Vafai]

(In reply to comment #4)
> Here are some variants (with blackslash included):
> 
> webkit.org/12345 WIN MAC DEBUG \ animations/stop-animation-on-suspend.html \ CRASH TEXT PASS
> webkit.org/12345 WIN MAC DEBUG [ animations/stop-animation-on-suspend.html ] CRASH TEXT PASS
> webkit.org/12345 WIN MAC DEBUG : animations/stop-animation-on-suspend.html : CRASH TEXT PASS
> webkit.org/12345 WIN MAC DEBUG ; animations/stop-animation-on-suspend.html ; CRASH TEXT PASS
> webkit.org/12345 WIN MAC DEBUG = animations/stop-animation-on-suspend.html = CRASH TEXT PASS
> webkit.org/12345 WIN MAC DEBUG - animations/stop-animation-on-suspend.html - CRASH TEXT PASS
> webkit.org/12345 WIN MAC DEBUG , animations/stop-animation-on-suspend.html , CRASH TEXT PASS
> webkit.org/12345 WIN MAC DEBUG . animations/stop-animation-on-suspend.html . CRASH TEXT PASS
> 
> I kind of dig backslash and square bracket versions. Those two separates tokens visually very well.

My vote would be backslash, but I declare bikeshed bankruptcy and will go along with any delimiter we choose.

(In reply to comment #5)
> By the way, if we already have 2 delimiters, then why not 3 as in?
> 
> webkit.org/12345 \ WIN MAC DEBUG \ animations/stop-animation-on-suspend.html \ CRASH TEXT PASS
> 
> Since there are logically 4 separate sections in this line.

I don't find I need the extra slash to easily visually delimit the bug number since it is pretty much always the first thing on the line and there are rarely two bugs for a given line. While I agree that this is the logical extension of separating each group, it seems like unnecessary overkill to me.

[Ryosuke Niwa]

(In reply to comment #6)
> (In reply to comment #4)
> > Here are some variants (with blackslash included):
> > 
> > webkit.org/12345 WIN MAC DEBUG \ animations/stop-animation-on-suspend.html \ CRASH TEXT PASS
> > webkit.org/12345 WIN MAC DEBUG [ animations/stop-animation-on-suspend.html ] CRASH TEXT PASS
> > webkit.org/12345 WIN MAC DEBUG : animations/stop-animation-on-suspend.html : CRASH TEXT PASS
> > webkit.org/12345 WIN MAC DEBUG ; animations/stop-animation-on-suspend.html ; CRASH TEXT PASS
> > webkit.org/12345 WIN MAC DEBUG = animations/stop-animation-on-suspend.html = CRASH TEXT PASS
> > webkit.org/12345 WIN MAC DEBUG - animations/stop-animation-on-suspend.html - CRASH TEXT PASS
> > webkit.org/12345 WIN MAC DEBUG , animations/stop-animation-on-suspend.html , CRASH TEXT PASS
> > webkit.org/12345 WIN MAC DEBUG . animations/stop-animation-on-suspend.html . CRASH TEXT PASS
> > 
> > I kind of dig backslash and square bracket versions. Those two separates tokens visually very well.
> 
> My vote would be backslash, but I declare bikeshed bankruptcy and will go along with any delimiter we choose.

Okay, let's go with backslash then! (unless someone objects)

> (In reply to comment #5)
> > By the way, if we already have 2 delimiters, then why not 3 as in?
> > 
> > webkit.org/12345 \ WIN MAC DEBUG \ animations/stop-animation-on-suspend.html \ CRASH TEXT PASS
> > 
> > Since there are logically 4 separate sections in this line.
> 
> I don't find I need the extra slash to easily visually delimit the bug number since it is pretty much always the first thing on the line and there are rarely two bugs for a given line. While I agree that this is the logical extension of separating each group, it seems like unnecessary overkill to me.

Fair enough.

[Dirk Pranke]

(In reply to comment #3)
> > For the record, though ...
> > > 
> > > 1. Make SLOW, SKIP and WONTFIX outcomes instead of modifiers.
> > 
> > Does SLOW imply PASS? We certainly have slow failures today, and if it doesn't imply pass, but only keeps its current semantics, it seems like it complicates the implementation (SLOW is now "special"). We could also just get rid of SLOW.
> 
> Yes. Slow needs to imply pass, unless you explicitly list another expected outcome as well. So, SLOW=>PASS, "SLOW TEXT"=>TEXT, "SLOW TEXT PASS"=>"TEXT PASS".
> 
> Specifically "SLOW TEXT" does not imply "SLOW PASS TEXT".
> 
> Personally, I prefer killing SLOW, but this has been controversial in the past and we've already had enough bikeshedding. :)
> 

Hm. I'm strongly tempted to kill SLOW instead of implement this :).

> > > 5. Use the same character as a separator before and after the test (":").
> > 
> > I'm not sure this is an improvement. If we were going to standardize on the same delimiter I would be open to using one that doesn't require a shift key as per request from rniwa. Possible candidates: "-", ";", ",", "=" (which I would prefer in that order, I think; frankly, I only really like "-" here, and I'm not sure I like any of them more than the existing ":"/"=" combo).
> 
> I don't really care what we use. Semi-colon seems fine to me. I've just seen it be a source of confusion that there are two different delimiters. 
> 

I quite like rniwa's brackets for this -- [] -- so that would be my vote. I'd probably want to stay away from commas since that starts to make the file look like CSV and that has all sorts of baggage. Frankly semicolons have some of the same issues. Avoiding any character that shows up in a layout test name already might be a good idea, which might rule out "-" and some of the others.

I also like the idea of allowing commas or more general glob-style expansion. It's a clean extension of the fact that we already allow directory prefixes, and can definitely cut down on the verbosity and maintenance for manuel entry. It might make automated updates more complicated, of course ...

I also agree w/ Ojan that I don't think we need to separate bug URLs out from the platform modifiers. URLs are distinctive enough.

[Tony Chang]

(In reply to comment #8)
> (In reply to comment #3)
> > > For the record, though ...
> > > > 
> > > > 1. Make SLOW, SKIP and WONTFIX outcomes instead of modifiers.
> > > 
> > > Does SLOW imply PASS? We certainly have slow failures today, and if it doesn't imply pass, but only keeps its current semantics, it seems like it complicates the implementation (SLOW is now "special"). We could also just get rid of SLOW.
> > 
> > Yes. Slow needs to imply pass, unless you explicitly list another expected outcome as well. So, SLOW=>PASS, "SLOW TEXT"=>TEXT, "SLOW TEXT PASS"=>"TEXT PASS".
> > 
> > Specifically "SLOW TEXT" does not imply "SLOW PASS TEXT".
> > 
> > Personally, I prefer killing SLOW, but this has been controversial in the past and we've already had enough bikeshedding. :)
> > 
> 
> Hm. I'm strongly tempted to kill SLOW instead of implement this :).

Please don't mix syntax changes with functionality changes in the same patch. If we want to change the meaning of anything, that should be in an isolated patch.

[Dirk Pranke]

(In reply to comment #9)
> > Hm. I'm strongly tempted to kill SLOW instead of implement this :).
> 
> Please don't mix syntax changes with functionality changes in the same patch. If we want to change the meaning of anything, that should be in an isolated patch.

Of course. I wouldn't be likely to make all of these changes in one patch, either. Dunno, I am torn on SLOW ...

[Ojan Vafai]

(In reply to comment #10)
> (In reply to comment #9)
> > > Hm. I'm strongly tempted to kill SLOW instead of implement this :).
> > 
> > Please don't mix syntax changes with functionality changes in the same patch. If we want to change the meaning of anything, that should be in an isolated patch.
> 
> Of course. I wouldn't be likely to make all of these changes in one patch, either. Dunno, I am torn on SLOW ...

If we got rid of slow, we'd need to change a few other things to compensate.
1. We'd need to increase the timeout considerably.
2. We'd need to decrease the timeout for tests expected to timeout in order to maintain buildbot cycle times.

Part 2 has been the contentious part of getting rid of SLOW since it will then always be hard to tell if the test would still timeout if it weren't marked TIMEOUT. Alternately, we could just skip tests that are expected to reliably TIMEOUT and have the longer timeout fo tests that timeout inconsistently. I'd be happy with that.

[Dirk Pranke]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > > Hm. I'm strongly tempted to kill SLOW instead of implement this :).
> > > 
> > > Please don't mix syntax changes with functionality changes in the same patch. If we want to change the meaning of anything, that should be in an isolated patch.
> > 
> > Of course. I wouldn't be likely to make all of these changes in one patch, either. Dunno, I am torn on SLOW ...
> 
> If we got rid of slow, we'd need to change a few other things to compensate.
> 1. We'd need to increase the timeout considerably.

Yeah, I was thinking we'd match the default timeout the other ports use (30s)

> 2. We'd need to decrease the timeout for tests expected to timeout in order to maintain buildbot cycle times.
>

I could see how this would be confusing. I'd be inclined to not do this.

I am wondering if there's really a point to running tests that TIMEOUT at all (or CRASH, for that matter). Maybe they should be skipped by default. The downside would be that you wouldn't be able to tell if a test started passing, and that's a pretty big downside.

Another idea I have considered is breaking the steps on the bot into a step that runs just the stable tests (no flaky tests, crashes, or timeouts), and have a second step or a second bot that runs the others. I don't think this is a big enough win to justify the complexity, though.

We should probably break the SLOW discussion off into an entirely different bug.

[Dirk Pranke]

(In reply to comment #8)
> I quite like rniwa's brackets for this -- [] -- so that would be my vote. 

looking at this again in the morning, now the brackets make it look like the test name should be optional. I'm changing my vote to backslash :).

[Dirk Pranke]

split off the SLOW discussion into its own bug: https://bugs.webkit.org/show_bug.cgi?id=86796

[Dirk Pranke]

closing as a duplicate of 89161, since that's where the action is :).

*** This bug has been marked as a duplicate of bug 89161 ***