Bug 111321 – editing/execCommand/toggle-unlink.html needs to support different expectations

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

[Claudio Saavedra]

Once bug 110487 is resolved, extending selection forward will yield different results depending on the platform. The test editing/execCommand/toggle-unlink.html currently doesn't support this, as the expected results are hardcoded into the script itself, redundantly.

[Claudio Saavedra]

Created attachment 191235 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 191235 [details]
Patch

The right fix is to make the behavior dependent on editing behavior and set editing behavior to mac in this test.

[Darin Adler]

Comment on attachment 191235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191235&action=review

> LayoutTests/ChangeLog:11
> +        (testSingleToggle): Just output the result of running the test and
> +        let the test infrastructure check whether the expectations are met
> +        or not.

We should not, then, write out the word PASS when the test may have failed.

[Claudio Saavedra]

(In reply to comment #2)
> (From update of attachment 191235 [details])
> The right fix is to make the behavior dependent on editing behavior and set editing behavior to mac in this test.

Wouldn't setting the editing behavior to Mac in this test be counter-productive? That would mean that, in this particular test, only the codepaths related to Mac would be tested, in all platforms. If the feature breaks for other platforms this test wouldn't catch it. How can that be better than having separate expectations for platforms that behave differently?

[Ryosuke Niwa]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 191235 [details] [details])
> > The right fix is to make the behavior dependent on editing behavior and set editing behavior to mac in this test.
> 
> Wouldn't setting the editing behavior to Mac in this test be counter-productive? That would mean that, in this particular test, only the codepaths related to Mac would be tested, in all platforms. If the feature breaks for other platforms this test wouldn't catch it. How can that be better than having separate expectations for platforms that behave differently?

You'll then add a new version of the test that tests Windows behavior.

[Claudio Saavedra]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 191235 [details] [details] [details])
> > > The right fix is to make the behavior dependent on editing behavior and set editing behavior to mac in this test.
> > 
> > Wouldn't setting the editing behavior to Mac in this test be counter-productive? That would mean that, in this particular test, only the codepaths related to Mac would be tested, in all platforms. If the feature breaks for other platforms this test wouldn't catch it. How can that be better than having separate expectations for platforms that behave differently?
> 
> You'll then add a new version of the test that tests Windows behavior.

This seems absolutely unnecessary to me. This is what different expectations per platform are for. The test shouldn't be hardcoding the expected outcome in its code. There are -expected.txt files that are meant to be used for that.

[Ryosuke Niwa]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> >
> > You'll then add a new version of the test that tests Windows behavior.
> 
> This seems absolutely unnecessary to me. This is what different expectations per platform are for. The test shouldn't be hardcoding the expected outcome in its code. There are -expected.txt files that are meant to be used for that.

That's what editing behaviors are for. We used to have all these platform-specific behaviors and it was an absolute nightmare to debug them because you needed access to that platform/port.

[Ryosuke Niwa]

Please grep setEditingBehaviors in LayoutTests/editing and see how we're testing various editing features.

[Claudio Saavedra]

(In reply to comment #8)
> Please grep setEditingBehaviors in LayoutTests/editing and see how we're testing various editing features.

I see. Then I'll add the suffix -mac.html to this test, set edditing behavior to Mac as you suggested, and add a new test for windows when landing bug 110487. Does that sound OK for you?

[Claudio Saavedra]

Created attachment 191456 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 191456 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191456&action=review

> LayoutTests/ChangeLog:14
> +        * editing/execCommand/script-tests/toggle-unlink-mac.js: Renamed
> +        from
> +        LayoutTests/editing/execCommand/script-tests/toggle-unlink.js.
> +        Also set editing behaviour to Mac.

Why was this file not svn mv'ed?

[Claudio Saavedra]

(In reply to comment #11)
> (From update of attachment 191456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191456&action=review
> 
> > LayoutTests/ChangeLog:14
> > +        * editing/execCommand/script-tests/toggle-unlink-mac.js: Renamed
> > +        from
> > +        LayoutTests/editing/execCommand/script-tests/toggle-unlink.js.
> > +        Also set editing behaviour to Mac.
> 
> Why was this file not svn mv'ed?

It is, to toggle-unlink-mac.js. The ChangeLog word wrapping might be a bit confusing here.

[Build Bot]

Comment on attachment 191456 [details]
Patch

Attachment 191456 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17013074

New failing tests:
editing/selection/selection-invalid-offset.html

[Claudio Saavedra]

(In reply to comment #13)
> (From update of attachment 191456 [details])
> Attachment 191456 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17013074
> 
> New failing tests:
> editing/selection/selection-invalid-offset.html

Failing test is unrelated to the patch as far as I can tell.. is it safe to commit?

[WebKit Review Bot]

Comment on attachment 191456 [details]
Patch

Clearing flags on attachment: 191456

Committed r144883: <http://trac.webkit.org/changeset/144883>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.