| Summary: | TestExpectation fallback is broken | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||
| Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | dpranke, jonlee, ojan, rniwa, simon.fraser | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Simon Fraser (smfr)
2012-12-20 15:10:13 PST
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 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. 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 ... We can just use the default baseline fallback paths for WK2 ports. There is no point in restricting search paths for TestExpectations here. Created attachment 180430 [details]
Patch
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? (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. (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. (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. (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 ... Comment on attachment 180430 [details] Patch Clearing flags on attachment: 180430 Committed r138314: <http://trac.webkit.org/changeset/138314> All reviewed patches have been landed. Closing bug. 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? (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. (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. (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? (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. |