Bug 48072 – test_darwin reads from my disk and logs

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

[Eric Seidel (no email)]

test_darwin reads from my disk and logs

test_darwin (webkitpy.layout_tests.run_webkit_tests_unittest.DryrunTest) ... 
FAILURES FOR PLATFORM: MAC-SNOWLEOPARD, BUILD_TYPE: RELEASE
Line:256 Path does not exist. LayoutTests/plugins/pass-different-npp-struct.html


FAILURES FOR PLATFORM: MAC-SNOWLEOPARD, BUILD_TYPE: RELEASE
Line:222 Path does not exist. LayoutTests/plugins/pass-different-npp-struct.html

ok

I assume it's reading from /tmp/layout-test-results?

[Adam Barth]

Proposal: Disable or remove all tests that touch the disk and/or network.

[Dirk Pranke]

test_darwin is supposed to read from your disk. It's testing that the darwin port of NRWT actually works. It's odd that you're getting errors; was there something wrong/changed in your checkout?

This is a variant of the http server starting up bug ... this actually intended to be an integration test, so I don't want to remove it completely. However it's fair that we should change when we want it to run.

I wouldn't expect it to be reading from /tmp/layout-test-results, though. Writing, yes.

[Adam Barth]

I think we need a separate integration test suite.  test-webkitpy is not intended to be for integration tests.

[Dirk Pranke]

(In reply to comment #3)
> I think we need a separate integration test suite.  test-webkitpy is not intended to be for integration tests.

The tests under webkitpy/layout_tests are there intentionally, and I want them to be run regularly, at least in conjunction with running new-run-webkit-tests as part of a test cycle (e.g., on bots that run new-webkit-tests). The rationale for this is that I don't want a bot to fail b/c of a bug in the code that is exposed by new-run-webkit-tests actually running the layout tests, and to not also see that it fails the regression suite for the code first.

It may be that they shouldn't be called *_unittest.py, and we need a separate script or bot step to run them.

What is your intent w/ test-webkitpy? When do you want it to be able to run, etc.? It seems like you want it to not fail on things that would actually cause NRWT (with some combination of parameters) to fail ?

[Adam Barth]

> What is your intent w/ test-webkitpy?

Maybe we should add run-webkitpy-unittests which is the subset of test-webkpy that are unit tests (e.g., no external dependencies).

> When do you want it to be able to run, etc.?

I'd like it to be fast and not make assumptions about the configuration / state of my system.

> It seems like you want it to not fail on things that would actually cause NRWT (with some combination of parameters) to fail ?

I'd love to be able to test enough of NRWT with unit tests to make that true.  However, there's a trade-off between some things and the external dependencies they introduce.  We've managed to test webkit-patch, commit-queue, the EWS, SheriffBot, and the style bot all with unit tests.  Now, we missing things from time to time, but the unit tests are hugely helpful.

[Dirk Pranke]

(In reply to comment #5)
> > What is your intent w/ test-webkitpy?
> 
> Maybe we should add run-webkitpy-unittests which is the subset of test-webkpy that are unit tests (e.g., no external dependencies).
>

Sure, this is an option.
 
> > When do you want it to be able to run, etc.?
> 
> I'd like it to be fast and not make assumptions about the configuration / state of my system.
>

And it is certainly possible to find a subset of the the layout_tests/* unittests that will meet this criteria (esp. after I land the filesystem changes), but not all of them will.
 
> > It seems like you want it to not fail on things that would actually cause NRWT (with some combination of parameters) to fail ?
> 
> I'd love to be able to test enough of NRWT with unit tests to make that true.  However, there's a trade-off between some things and the external dependencies they introduce.  We've managed to test webkit-patch, commit-queue, the EWS, SheriffBot, and the style bot all with unit tests.  Now, we missing things from time to time, but the unit tests are hugely helpful.

There will always be some amount of failures that NRWT will trip over that unit tests that don't ever look at the real files won't. Which isn't to say that unit tests for NRWT can't catch the bulk of the mistakes. 

I think we're in violent agreement over all of this, we just don't necessarily agree on where to draw the lines in a particular environment.

Do you imagine different levels of testing happening (a) manually, (b) at presubmit time, (c) on the EWS bots, and (d) on the regular bots? If so, what happens where? 

At the very least, I would like (c) == (d) so that if something passes (c) it should pass (d). I would also like (d) to contain some amount of integration tests, although it doesn't necessarily have to include every test that could be run manually.

[Adam Barth]

test-webkitpy used to finish in a few seconds.  Now it takes minutes.  I'd like get it back to seconds.  Also, I'd like to be able to run test-webkitpy in parallel with the commit-queue running on my machine, which also runs test-webkitpy, writes to temp folder, and spins up an HTTP server.  Finally, I'd like test-webkitpy not to dump files in my working copy.  AFACT, all these problem are caused by running tests that have external dependencies, such as test_darwin.

[Dirk Pranke]

Created attachment 73709 [details]
patch too big, needs to be broken up

[Dirk Pranke]

I'm going to use this as a meta-bug for tracking all of the remaining cleanup to get all of the webkitpy.layout_tests.* classes using the proper fake filesystems etc.

The patch I've just uploaded covers it all (test-webkitpy now takes < 4 seconds!) but it needs to be cleaned up a bit and probably broken into chunks to be easier to review. 

I had to add a bunch more entrypoints to common.system.filesystem, and I renamed filesystem_mock to common.in_memory_filesystem, since it really wasn't a mock object any more (if it ever was).

There's still a few places in the code using os.sep, which need to be replaced by a filesystem.sep property. 

I think this should get most of the layout_tests/* tests running under win32 once that happens as well.

[Dirk Pranke]

Created attachment 73940 [details]
Patch

[Eric Seidel (no email)]

This test also depends on having a build of WebKIt, which is wrong:

test_darwin (webkitpy.layout_tests.run_webkit_tests_unittest.DryrunTest) ... sh: line 1: 66217 Trace/BPT trap          /Projects/WebKit/WebKitBuild/Debug/DumpRenderTree --print-supported-features 2>&1
sh: line 1: 66290 Trace/BPT trap          /Projects/WebKit/WebKitBuild/Debug/DumpRenderTree --print-supported-features 2>&1
ok

We should just remove the test.

[Dirk Pranke]

No, we shouldn't remove the test. It's an integration test, we just need to figure out how to run it only when it's appropriate to do so, and disable it otherwise.

[Dirk Pranke]

Okay, the patches attached to this bug are now officially irrelevant. With the exception of bug 52683, which disables test_darwin and a test in port_testcase that attempts to spawn ImageDiff if it exists, all of the other unit tests no longer rely on anything in the filesystem and no longer attempt to grab exclusive resources (by default).

We still need a way to do integration tests, but I want to track that as a separate issue so that we can close these bugs and start fresh.

[Simon Fraser (smfr)]

Why does this depend on bug 52683?

[Dirk Pranke]

(In reply to comment #14)
> Why does this depend on bug 52683?

Whoops. Wrong bug. I meant bug 52863.