Bug 105583 – TestExpectation fallback is broken

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

[Simon Fraser (smfr)]

http/tests/notifications is skipped in platform/mac-lion/TestExpectations but doesn't occur in any other TestExpectaions file.

On Mac Lion, the fallback path shows as:

13:48:20.439 89663 Test configuration: <lion, x86_64, release>
13:48:20.439 89663 Placing test results in /Volumes/Data/slave/lion-release-tests-wk2/build/layout-test-results
13:48:20.439 89663 Baseline search path: mac-wk2 -> mac-lion -> mac -> generic

but these tests are still running.

webkitpy is so opaque that I wasn't able to figure out why.

[Simon Fraser (smfr)]

An example of these tests getting run:

http://build.webkit.org/builders/Apple%20Lion%20Release%20WK2%20%28Tests%29/builds/6513/steps/layout-test/logs/stdio

[Simon Fraser (smfr)]

So on Lion, running r-w-t -2 parses:

LayoutTests/platform/mac/TestExpectations
LayoutTests/platform/mac-wk2/TestExpectations
LayoutTests/platform/wk2/TestExpectations

Running without -2 parses:
LayoutTests/platform/mac/TestExpectations
LayoutTests/platform/mac-lion/TestExpectations

This is clearly broken.

[Dirk Pranke]

argh. the whole mac-lion file is being skipped on the -wk2 port. looks like I broke this when I added the support for the -wk2 port names. fixing now ...

[Ryosuke Niwa]

We can just use the default baseline fallback paths for WK2 ports. There is no point in restricting search paths for TestExpectations here.

[Dirk Pranke]

Created attachment 180430 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 180430 [details]
Patch

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

> Tools/ChangeLog:15
> +        * Scripts/webkitpy/layout_tests/port/port_testcase.py:

No new tests?

[Dirk Pranke]

(In reply to comment #4)
> We can just use the default baseline fallback paths for WK2 ports. There is no point in restricting search paths for TestExpectations here.

I'm not sure I follow you; are you suggesting we should use the same fallback path for expectations files that we do for baselines?

(In reply to comment #6)
> (From update of attachment 180430 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180430&action=review
> 
> > Tools/ChangeLog:15
> > +        * Scripts/webkitpy/layout_tests/port/port_testcase.py:
> 
> No new tests?

There were existing tests that attempted to cover this case, but they were doing it wrong (manipulating internal flags like self._name rather than using the public APIs). I changed the existing tests to parse the port names properly and that catches the problem.

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #4)
> > We can just use the default baseline fallback paths for WK2 ports. There is no point in restricting search paths for TestExpectations here.
> 
> I'm not sure I follow you; are you suggesting we should use the same fallback path for expectations files that we do for baselines?
> 

Historically we used a different list for Skipped files than we did for baselines, that was because there was no way to "un-skip" a file (so if you skipped a file in platform/mac, it would also be skipped regardless of what is in platform/mac-lion). You can un-skip things in TestExpectation, so your suggestion would be fine. I don't have a strong opinion here (although I think consistency would be good so would probably agree that that's the right thing to do), but it's up to the folks who use multiple files to chime in.

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #4)
> > We can just use the default baseline fallback paths for WK2 ports. There is no point in restricting search paths for TestExpectations here.
> 
> I'm not sure I follow you; are you suggesting we should use the same fallback path for expectations files that we do for baselines?

Yes. I think we're adding too much complexity here.

> (In reply to comment #6)
> > No new tests?
> 
> There were existing tests that attempted to cover this case, but they were doing it wrong (manipulating internal flags like self._name rather than using the public APIs). I changed the existing tests to parse the port names properly and that catches the problem.

Okay. Thanks for the clarification.

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #4)
> > > We can just use the default baseline fallback paths for WK2 ports. There is no point in restricting search paths for TestExpectations here.
> > 
> > I'm not sure I follow you; are you suggesting we should use the same fallback path for expectations files that we do for baselines?
> 
> Yes. I think we're adding too much complexity here.
> 

As I said, fine by me. I'll post a separate patch for that and affected folks can chime in if they do or don't want the change. I'll land this patch now to fix the tree, though ...

[Dirk Pranke]

Comment on attachment 180430 [details]
Patch

Clearing flags on attachment: 180430

Committed r138314: <http://trac.webkit.org/changeset/138314>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

It's also distressing that the "Baseline search path: mac-wk2 -> mac-lion -> mac -> generic" output was incorrect, and didn't reflect which expectations files were actually being parsed. Is that fixed with the patch?

[Ryosuke Niwa]

(In reply to comment #13)
> It's also distressing that the "Baseline search path: mac-wk2 -> mac-lion -> mac -> generic" output was incorrect, and didn't reflect which expectations files were actually being parsed. Is that fixed with the patch?

That's expected. The baseline search path was correct. The underlying problem here is that the baselinse search paths is DIFFERENT from search paths for TestExpectations files, which is mind blowingly counter intuitive to say the least.

I think we should change it to match the baseline search paths.

[Dirk Pranke]

(In reply to comment #14)
> (In reply to comment #13)
> > It's also distressing that the "Baseline search path: mac-wk2 -> mac-lion -> mac -> generic" output was incorrect, and didn't reflect which expectations files were actually being parsed. Is that fixed with the patch?
> 
> That's expected. The baseline search path was correct. The underlying problem here is that the baselinse search paths is DIFFERENT from search paths for TestExpectations files, which is mind blowingly counter intuitive to say the least.
> 
> I think we should change it to match the baseline search paths.

Right.

[Simon Fraser (smfr)]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > It's also distressing that the "Baseline search path: mac-wk2 -> mac-lion -> mac -> generic" output was incorrect, and didn't reflect which expectations files were actually being parsed. Is that fixed with the patch?
> > 
> > That's expected. The baseline search path was correct. The underlying problem here is that the baselinse search paths is DIFFERENT from search paths for TestExpectations files, which is mind blowingly counter intuitive to say the least.
> > 
> > I think we should change it to match the baseline search paths.
> 
> Right.

Is there a bug to track that?

[Ryosuke Niwa]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > It's also distressing that the "Baseline search path: mac-wk2 -> mac-lion -> mac -> generic" output was incorrect, and didn't reflect which expectations files were actually being parsed. Is that fixed with the patch?
> > > 
> > > That's expected. The baseline search path was correct. The underlying problem here is that the baselinse search paths is DIFFERENT from search paths for TestExpectations files, which is mind blowingly counter intuitive to say the least.
> > > 
> > > I think we should change it to match the baseline search paths.
> > 
> > Right.
> 
> Is there a bug to track that?

Has been fixed in https://bugs.webkit.org/show_bug.cgi?id=105599.