nrwt: fix the dryrun and mock ports to work w/ DriverProxy, reftests
Created attachment 124237 [details] Patch
The code in MockDRT port to override the command line is particularly ugly. I am open to suggestions for improving it. I hope to land a bunch of changes to test-webkitpy soon that will allow this code to be exercised reliably on the bots, at which point we'll be able to get much better coverage of webkit.py, server_process.py, chromium.py, etc.
Why do we want this code? Ojan seems to be testing perf w/o these working?
(I'm not saying we don't want it. But we should have justification as to why we want it fixed isntead of removing it.)
Understood; these are good questions ... First, I'm not actually convinced that dryrun.py is still particularly useful; it predates mock_drt.py and I'm not sure that running it tells me anything than mock_drt won't, so we could probably get rid of it. mock_drt, on the other hand, serves (or will serve) a few purposes. First, it captures in python code how exactly we expect DRTs to behave, so it's good documentation for the protocols (easier to read than the corresponding C++ code). If something breaks or acts weird on the bots, it allows me to pretty quickly triage whether the problem is likely to be in DRT or in NRWT (or in some interaction). I've found a bunch of subtle bugs this way. Second, it allows us to write integration tests that will exercise the port code in a way that is pretty clear and obvious. This will allow us to get good test coverage from black-box tests (which as you've probably learned I generally prefer over white box unit tests as long as they're not slow and don't affect the environment). I think having a "real" MockDRT object will be almost as useful as the MockFileSystem has been. Third, it does provide a realistic end-to-end test for benchmarking (assuming a DRT that is as fast as one could expect it to be), and can be used to benchmark running tests in parallel that are actually exercising the file system, IPC, etc. For me, in tuning how well the manager/worker code works, it's been worth it for this alone. Ojan's benchmarking is testing other parts of the NRWT code base (how fast everything but running the test is). It's very useful as well, but not at all the same.
Comment on attachment 124237 [details] Patch cancelling review - I'm going to delete the dryrun port and rework the mock port.
Thank you Dirk. Sorry for being slow to respond this morning.
Created attachment 124660 [details] remove dryrun, clean up mock_drt
Comment on attachment 124660 [details] remove dryrun, clean up mock_drt View in context: https://bugs.webkit.org/attachment.cgi?id=124660&action=review > Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:49 > +# Since we execute this script directly as part of the unit tests, we need to ensure > +# that Tools/Scripts is in sys.path for the next imports to work correctly. > +script_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))) > +if script_dir not in sys.path: > + sys.path.append(script_dir) Why are we doing more of these? > Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:166 > + self.assertEqual(stderr.getvalue(), '' if options.chromium else '#EOF\n') :( Why?
Comment on attachment 124660 [details] remove dryrun, clean up mock_drt View in context: https://bugs.webkit.org/attachment.cgi?id=124660&action=review >> Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py:49 >> + sys.path.append(script_dir) > > Why are we doing more of these? This code would've always been necessary, but perhaps I was the only one running this code and I always have the Scripts dir in my PYTHONPATH. I could split the executable script out and move it into Tools/Scripts, but I thought it was somewhat better to have everything in one file. >> Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:166 >> + self.assertEqual(stderr.getvalue(), '' if options.chromium else '#EOF\n') > > :( Why? Chromium never writes anything to stderr by default, but apparently the Apple DRT port does (which surprises me; I need to look into that further).
Comment on attachment 124660 [details] remove dryrun, clean up mock_drt View in context: https://bugs.webkit.org/attachment.cgi?id=124660&action=review > Tools/Scripts/webkitpy/layout_tests/port/base.py:698 > + filename = self._filesystem.abspath(filename) > assert filename.startswith(self.layout_tests_dir()), "%s did not start with %s" % (filename, self.layout_tests_dir()) > return filename[len(self.layout_tests_dir()) + 1:] > > def relative_perf_test_filename(self, filename): > + filename = self._filesystem.abspath(filename) What is our normal convention for paths? Relative to the webkit root? absolute?
(In reply to comment #11) > (From update of attachment 124660 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124660&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:698 > > + filename = self._filesystem.abspath(filename) > > assert filename.startswith(self.layout_tests_dir()), "%s did not start with %s" % (filename, self.layout_tests_dir()) > > return filename[len(self.layout_tests_dir()) + 1:] > > > > def relative_perf_test_filename(self, filename): > > + filename = self._filesystem.abspath(filename) > > What is our normal convention for paths? Relative to the webkit root? absolute? I don't know that we have a "normal" convention across all of webkit.py; however, these routines are designed to take full paths and return paths relative to Source/LayoutTests. I need to add the abspath to account for some changes Ojan made in bug 77170 that I didn't fully understand (I think). I can go back and remove those and see what breaks (I've forgotten at the moment).
Comment on attachment 124660 [details] remove dryrun, clean up mock_drt View in context: https://bugs.webkit.org/attachment.cgi?id=124660&action=review >>> Tools/Scripts/webkitpy/layout_tests/port/base.py:698 >>> + filename = self._filesystem.abspath(filename) >> >> What is our normal convention for paths? Relative to the webkit root? absolute? > > I don't know that we have a "normal" convention across all of webkit.py; however, these routines are designed to take full paths and return paths relative to Source/LayoutTests. I need to add the abspath to account for some changes Ojan made in bug 77170 that I didn't fully understand (I think). I can go back and remove those and see what breaks (I've forgotten at the moment). Are these routines which are only used by the mock port? I'm confused why these fixes would be needed for this patch?
Comment on attachment 124660 [details] remove dryrun, clean up mock_drt View in context: https://bugs.webkit.org/attachment.cgi?id=124660&action=review > Tools/ChangeLog:13 > + never worked w/ reftests. Since we don't exercise this module in Nit: please spell-out "with".
Comment on attachment 124660 [details] remove dryrun, clean up mock_drt View in context: https://bugs.webkit.org/attachment.cgi?id=124660&action=review >>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:698 >>>> + filename = self._filesystem.abspath(filename) >>> >>> What is our normal convention for paths? Relative to the webkit root? absolute? >> >> I don't know that we have a "normal" convention across all of webkit.py; however, these routines are designed to take full paths and return paths relative to Source/LayoutTests. I need to add the abspath to account for some changes Ojan made in bug 77170 that I didn't fully understand (I think). I can go back and remove those and see what breaks (I've forgotten at the moment). > > Are these routines which are only used by the mock port? I'm confused why these fixes would be needed for this patch? No, they are used elsewhere. You are asking good questions; I'm thinking that maybe I developed this patch on a machine where I had some symlinks that were otherwise confusing things, but if I delete these lines now, everything still seems to work, so I will delete them.
Created attachment 124837 [details] remove abspath call, fix nit in changelog
Comment on attachment 124837 [details] remove abspath call, fix nit in changelog OK. Thanks.
Committed r106416: <http://trac.webkit.org/changeset/106416>