Bug 39433 – editingBehavior settings needs to be set back to a reasonable default between tests

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

[Martin Robinson]

The actual setting depends on the platform.

[Martin Robinson]

Created attachment 56608 [details]
Patch

[Martin Robinson]

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

[Martin Robinson]

Created attachment 56616 [details]
Patch

[Martin Robinson]

Reverted tests causing failures until this can be sorted out properly: http://trac.webkit.org/changeset/59852

[Martin Robinson]

Committed r59857: <http://trac.webkit.org/changeset/59857>

[WebKit Review Bot]

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

[Martin Robinson]

Created attachment 56624 [details]
Reset Mac and Windows to platform default between tests

[Eric Seidel (no email)]

Comment on attachment 56624 [details]
Reset Mac and Windows to platform default between tests

LGTM!

[Martin Robinson]

Committed r59861: <http://trac.webkit.org/changeset/59861>

[Alexey Proskuryakov]

> 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?

[Martin Robinson]

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+.

[Alexey Proskuryakov]

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.

[Antonio Gomes]

(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

[Antonio Gomes]

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

[Kent Tamura]

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.

[Eric Seidel (no email)]

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?

[WebKit Review Bot]

Attachment 56970 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2315575

[Antonio Gomes]

(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

[Antonio Gomes]

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>

[Antonio Gomes]

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>

[WebKit Review Bot]

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

[Ojan Vafai]

(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.

[Alexey Proskuryakov]

> 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.

[Antonio Gomes]

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

[Ojan Vafai]

(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.

[Antonio Gomes]

(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.

[Antonio Gomes]

(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.

[Martin Robinson]

Committed r60865: <http://trac.webkit.org/changeset/60865>

[Antonio Gomes]

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

[Antonio Gomes]

> 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.

Filed bug 48130 and bug 48131 about these two.