| Summary: | editingBehavior settings needs to be set back to a reasonable default between tests | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Martin Robinson <mrobinson> |
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | abarth, ap, dglazkov, eric, ojan, tonikitoo, webkit.review.bot |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | All | ||
| Bug Depends on: | |||
| Bug Blocks: | 38603 | ||
| Attachments: | |||
|
Description
Martin Robinson
2010-05-20 10:52:21 PDT
Created attachment 56608 [details]
Patch
Comment on attachment 56608 [details]
Patch
via IRC:
(10:57:24 AM) ojan: mrobinson: i'd rather see a shared method in Settings
(10:57:53 AM) ojan: mrobinson: that way, if a port changes which editing behavior they use ona platform, they only change it in one place
(11:01:09 AM) ojan: mrobinson: so, i'm thinking...add Settings::resetEditingBehavior and move the logic from the Settings constructor into that method. then call the method instead of setEditingBehavior.
(11:05:48 AM) ojan: so, i'm just staying to move those 7 lines of code into Settings::resetEditingBehavior and instead call that method from the constructor
Created attachment 56616 [details]
Patch
Reverted tests causing failures until this can be sorted out properly: http://trac.webkit.org/changeset/59852 Committed r59857: <http://trac.webkit.org/changeset/59857> http://trac.webkit.org/changeset/59852 might have broken Tiger Intel Release The following changes are on the blame list: http://trac.webkit.org/changeset/59850 http://trac.webkit.org/changeset/59851 http://trac.webkit.org/changeset/59852 http://trac.webkit.org/changeset/59853 http://trac.webkit.org/changeset/59854 Created attachment 56624 [details]
Reset Mac and Windows to platform default between tests
Comment on attachment 56624 [details]
Reset Mac and Windows to platform default between tests
LGTM!
Committed r59861: <http://trac.webkit.org/changeset/59861> > Committed r59861: <http://trac.webkit.org/changeset/59861>
Sounds like this should be closed.
+ editingBehavior settings needs to be set back to a reasonable default between tests
FWIW, I'd expect "reasonable default" to be the same on all platforms. What's the value in making it different?
This is required because many tests still exist which expect different behavior on different platforms (detected via user agent). At some point, if they are all rewritten, we can probably set some behavior to be the default in DRT. This still needs to be fixed for Chromium and GTK+. How many tests like that are there? What we basically need is make two or three copies of each, and set different behaviors in those. (In reply to comment #12) > How many tests like that are there? What we basically need is make two or three copies of each, and set different behaviors in those. ~14 tests in /editing. $ grep -nHR navigator.userAgent.search LayoutTests/editing/ - LayoutTests/editing/selection/script-tests/click-in-padding-with-multiple-line-boxes.js - LayoutTests/editing/selection/script-tests/click-in-margins-inside-editable-div.js - LayoutTests/editing/selection/script-tests/shift-click.js - LayoutTests/editing/selection/extend-selection-after-double-click.html - LayoutTests/editing/selection/extend-after-mouse-selection.html - LayoutTests/editing/selection/move-begin-end.html - LayoutTests/editing/selection/5195166-1.html - LayoutTests/editing/input/option-page-up-down.html - LayoutTests/editing/undo/undo-deleteWord.html - LayoutTests/editing/deleting/delete-by-word-001.html - LayoutTests/editing/deleting/delete-by-word-002.html Created attachment 56970 [details] (committed with r60158, reviewed by eseidel, tkent) Reset Gtk and Chromium to platform default between tests according to comment 11, it seems needed Comment on attachment 56970 [details] (committed with r60158, reviewed by eseidel, tkent) Reset Gtk and Chromium to platform default between tests Chromium part looks OK. Comment on attachment 56970 [details] (committed with r60158, reviewed by eseidel, tkent) Reset Gtk and Chromium to platform default between tests Looks OK to me. Why doesn't this affect tests? Can we write a test this would affect? Attachment 56970 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2315575 (In reply to comment #17) > Attachment 56970 [details] did not build on chromium: > Build output: http://webkit-commit-queue.appspot.com/results/2315575 s/EditingBehaviorWindows/EditingBehaviorWin/g , will fix before land. (In reply to comment #16) > (From update of attachment 56970 [details]) > Looks OK to me. Why doesn't this affect tests? Can we write a test this would affect? @Eric: it will affect tests, for example as soon as https://bugs.webkit.org/attachment.cgi?id=56608 re-lands and all tests cited in comment #12 got changed to use the setEditingBehaviour API of LayoutTestController instead of the user agent string Comment on attachment 56970 [details] (committed with r60158, reviewed by eseidel, tkent) Reset Gtk and Chromium to platform default between tests Clearing flags on attachment: 56970 Committed r59861: <http://trac.webkit.org/changeset/59861> Comment on attachment 56970 [details] (committed with r60158, reviewed by eseidel, tkent) Reset Gtk and Chromium to platform default between tests err: Committed r60158: <http://trac.webkit.org/changeset/60158> http://trac.webkit.org/changeset/60158 might have broken GTK Linux 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/60158 http://trac.webkit.org/changeset/60159 (In reply to comment #10) > + editingBehavior settings needs to be set back to a reasonable default between tests > > FWIW, I'd expect "reasonable default" to be the same on all platforms. What's the value in making it different? For non-DRT, the code in Settings.cpp currently has a different default for Mac and everyone else. So, DRT should, at reset the value to that default. I wouldn't want DRT always defaulting to EditingMacBehavior because then we lose test coverage. For example, if someone accidentally modifies the EditingWinBehavior behavior, then only the tests that call setEditingBehavior would actually test that. I think it's unfortunate that we have setEditingBehavior calls littered throughout the code though. I'd prefer that we had Settings::resetEditingBehavior or Settings::defaultEditingBehavior so that we have the logic of which editing behavior to use in one place. Eric pushed back on this on #webkit saying that this would be different than all other DRT setters. I don't have enough experience hacking on DRT to say. (In reply to comment #12) > How many tests like that are there? What we basically need is make two or three copies of each, and set different behaviors in those. Now that we've exposed setEditingBehavior, we can modify those tests to test both EditingMacBehavior and EditingWinBehavior. There's no need for them to have platform specific expectations now. > I wouldn't want DRT always defaulting to EditingMacBehavior because then we lose test coverage.
I disagree with that. We need reproducible results more than we need accidental testing - running tests on Mac should be as close to running them on other platforms as possible.
I think now that Gtk, Mac, Win, Chromium and soon Qt (see bug 39680) are in, we can revert back r59852 and r59857, and gradually adjust other call sites (see comment #13) to use the new setEditingBehavior. ... and a some point close this bug as FIXED (In reply to comment #23) > > I wouldn't want DRT always defaulting to EditingMacBehavior because then we lose test coverage. > > I disagree with that. We need reproducible results more than we need accidental testing - running tests on Mac should be as close to running them on other platforms as possible. If a test fails on Windows because of the editing behavior, then one of two things should happen: 1. There's a bug when using EditingWinBehavior and that should be fixed. 2. The test unintentionally is testing platform-specific behavior and the test should be fixed to test both EditingMacBehavior and EditingWinBehavior. As long as the above happens, running tests from a Mac is fairly close to running them on other platforms. A given commit might cause a failure on a non-mac port, but the fix will mean broader test coverage. We have bots specifically to ensure that the port-specific code is tested. I don't see why it makes sense for platform-specific code to not get the same testing. (In reply to comment #24) > I think now that Gtk, Mac, Win, Chromium and soon Qt (see bug 39680) are in, we can revert back r59852 and r59857, and gradually adjust other call sites (see comment #13) to use the new setEditingBehavior. @Martin, plans to re-land these two backed out revisions? they should be safe to go now. (In reply to comment #25) > (In reply to comment #23) > > > I wouldn't want DRT always defaulting to EditingMacBehavior because then we lose test coverage. If all platform specific editing tests are going through all code paths possible then it would make sense to default DRT generally to the same editing behavior type (being it "mac" or not). However the condition is not what happens currentl, and due to that I also agree that as tests are today, having them all sharing the same default would lose coverage. That is indeed something that could change in the future though, and I could have a look at that. For example, if we had at least one layout test for each of the platform specific editing behaviors, and each test were going through all code paths, then we would be safe to default all DRTs to the same value. Committed r60865: <http://trac.webkit.org/changeset/60865> Yesterday, I started converting all tests relying on platform specific (for now, Mac x Win) behavior to run with the LayoutTestController::setEditingBehavior machinery. I've fixed the following ones so far: - bug 47468: editing/selection/extend-after-mouse-selection.html - bug 47471: editing/selection/script-tests/click-in-margins-inside-editable-div.js - bug 47472: editing/selection/script-tests/click-in-padding-with-multiple-line-boxes.js It basically means that instead of having mac, win and qt expected results, we now have only one cross platform expected result file, since all platform behaviors are being tested. I will continue this work along the week. As a side note, it would be good to turn tests like editing/deleting/delete-by-word-001.html and editing/deleting/delete-by-word-002.html to not dump the render tree, but dump as text. I will try this as well. > (In reply to comment #12) > > How many tests like that are there? What we basically need is make two or three copies of each, and set different behaviors in those. > > ~14 tests in /editing. > > $ grep -nHR navigator.userAgent.search LayoutTests/editing/ > - LayoutTests/editing/selection/script-tests/click-in-padding-with-multiple-line-boxes.js > - LayoutTests/editing/selection/script-tests/click-in-margins-inside-editable-div.js > - LayoutTests/editing/selection/script-tests/shift-click.js > - LayoutTests/editing/selection/extend-selection-after-double-click.html > - LayoutTests/editing/selection/extend-after-mouse-selection.html > - LayoutTests/editing/selection/move-begin-end.html > - LayoutTests/editing/selection/5195166-1.html > - LayoutTests/editing/input/option-page-up-down.html > - LayoutTests/editing/undo/undo-deleteWord.html > - LayoutTests/editing/deleting/delete-by-word-001.html > - LayoutTests/editing/deleting/delete-by-word-002.html |