Bug 217548 – REGRESSION (r267761): editing/mac/spelling/autocorrection-contraction.html is a constant timeout on macOS wk2 Debug

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Hector Lopez]

editing/mac/spelling/autocorrection-contraction.html

Test is a constant timeout according to history on macOS wk2 Debug. First occurrence of a timeout is at r267762.

History:

https://results.webkit.org/?suite=layout-tests&test=editing%2Fmac%2Fspelling%2Fautocorrection-contraction.html

Please see attached for stderr log.

[Hector Lopez]

Created attachment 410990 [details]
stderr

[Alexey Proskuryakov]

Looking at test history, I think that https://trac.webkit.org/r267761 is the more likely culprit.

Darin, will you have the time to look into this?

[Radar WebKit Bug Importer]

<rdar://problem/70162138>

[Hector Lopez]

Test expectation while while investigated:

https://trac.webkit.org/changeset/268305/webkit

[Darin Adler]

Yes, I’ll figure out what happened and fix it.

[Darin Adler]

This "null is not an object" stuff is affecting more than one test!

[Darin Adler]

May be a challenge to fix if I can’t reproduce it locally though.

[Darin Adler]

Very strange. This is same expectations change is in r267997. Did it get changed back?

[Darin Adler]

Oh, I see, it’s happening on all builds, not just BigSur+.

[Darin Adler]

Created attachment 411027 [details]
Patch

[Darin Adler]

The patch looks large because it *removes* two expected PNG image files and other expected files. The actual change is small.

[Alexey Proskuryakov]

Comment on attachment 411027 [details]
Patch

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

Nice!

> Tools/ChangeLog:9
> +        (ShouldBuildTest): Removed test case that depended on a file that is now deleted.

Can it be changed to use another file, that's not deleted? There are other -expected.txt in this directory.

At some point, we should remove all tests from LayoutTests/platform to avoid the confusing situation with platform-specific results for platform-specific tests, but until then, tests are helpful to keep some sanity.

> Tools/TestRunnerShared/Bindings/JSBasics.cpp:92
> +    return value && JSValueIsObject(context, value) ? const_cast<JSObjectRef>(value) : nullptr;

I would rewrite this without a ternary operator, or at the very least, would use parentheses around the condition. I don't think that saving two bytes is worth decreased readability.

> LayoutTests/ChangeLog:9
> +        * editing/mac/spelling/autocorrection-contraction-expected.png: Removed.

The removed PNG shows that the original test also included "wouldn" and "wouldn'", also without underlines. Do you know what's up with that? Does the test even work at all?

[Darin Adler]

Comment on attachment 411027 [details]
Patch

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

>> Tools/ChangeLog:9
>> +        (ShouldBuildTest): Removed test case that depended on a file that is now deleted.
> 
> Can it be changed to use another file, that's not deleted? There are other -expected.txt in this directory.
> 
> At some point, we should remove all tests from LayoutTests/platform to avoid the confusing situation with platform-specific results for platform-specific tests, but until then, tests are helpful to keep some sanity.

I’ll just omit this change. I figured out how to run the test and confirmed that the test doesn’t rely on files actually being there. So I don’t need to touch it at this time.

>> Tools/TestRunnerShared/Bindings/JSBasics.cpp:92
>> +    return value && JSValueIsObject(context, value) ? const_cast<JSObjectRef>(value) : nullptr;
> 
> I would rewrite this without a ternary operator, or at the very least, would use parentheses around the condition. I don't think that saving two bytes is worth decreased readability.

This shows that "readability" is in the eye of the beholder. I like this version and find it more readable than the ones that you suggest. I agree that if the expression was longer or more complex it would be less readable than a separate if statement. I also agree that if unfamiliar with trinary expressions, someone might prefer a style that involves more parentheses.

I didn’t omit parentheses to "save two bytes": I omitted them because I find this more readable without them, and slightly irritating with them.

Given that this is subjective, I’m not sure whether to just go with your style preference or stick with my own.

I think for now I will leave this as is, but I am not repudiating your suggestions, simply disagreeing with them and thus uncertain what I should do.

>> LayoutTests/ChangeLog:9
>> +        * editing/mac/spelling/autocorrection-contraction-expected.png: Removed.
> 
> The removed PNG shows that the original test also included "wouldn" and "wouldn'", also without underlines. Do you know what's up with that? Does the test even work at all?

I know what’s up with that, and it’s a little complicated:

The spirit of this test, intended to check that valid words in contraction form don’t get underlines, does not require typing "wouldn" and "wouldn'" to trigger automatic correction. But the test *did* include those before. For reasons explained below, I removed them.

Since the point of the test is to verify the absence of spelling checking and autocorrection underlines, running it as a non-pixel test is worthless. Because of that, the test has *not* been testing what it’s supposed to test in a long time except when we run pixel tests and take pixel-only test failures seriously.

The test, with different expectations for mac-wk2, *has* been demonstrating a difference between legacy and modern WebKit on Mac. In legacy WebKit, autocorrect has been changing "wouldn" to "would" and "wouldn'" to "would'". But under modern WebKit autocorrect is not performing those corrections. The test has been verifying that, but that is not what it was intended for. It would be OK to add a new test covering that, but it’s not critical to have *this* test continue to test that rather than what it was originally designed for. Nor is it testing much under modern WebKit. But I suppose it is making sure we don’t regress in legacy WebKit, and lose that behavior on these two particular misspelled words. That part of the test was removed/lost, but there are plenty of other autocorrection tests.

Currently we can’t use a reference test to check for the *presence* of spelling underlines, but we can use it to check for the *absence* of them.

Given all of that, I decided to make *this* test cover at the core what it was originally intended for reliably, and do that as a reference test. By using an element in the expected file that is not editable (I made that revision after what you reviewed here, removed the contenteditable attribute), it’s a reliable reference test that will fail if underlines show up. The test does still test that we don’t accidentally add autocorrection underlines when typing a properly spelled contraction. The original test does have examples intended to cause autocorrection, and I suppose that covers one more where we want to be sure that we lack autocorrection underlines, but it’s not critical, and at this time it would be difficult to include a case that intentionally results in a spelling check underline but not autocorrection underline in a reference test. If we do add that, it should be a separate test, because otherwise we obscure the correct results of the other part of the test, under modern WebKit.

I think it would be valuable to create more tests. If we don’t already have them, we should create autocorrection tests demonstrating that autocorrection works in legacy WebKit and sadly I think they will show that it does not in modern WebKit. We could also come up with a way to reliably test for the *presence* of autocorrection and spelling underlines, probably by adding an API to WebKit internals that allows us to explicitly add those underlines, so we can call that in the reference file.

[Darin Adler]

Committed r268321: <https://trac.webkit.org/changeset/268321>

[Darin Adler]

Comment on attachment 411027 [details]
Patch

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

>>> Tools/ChangeLog:9
>>> +        (ShouldBuildTest): Removed test case that depended on a file that is now deleted.
>> 
>> Can it be changed to use another file, that's not deleted? There are other -expected.txt in this directory.
>> 
>> At some point, we should remove all tests from LayoutTests/platform to avoid the confusing situation with platform-specific results for platform-specific tests, but until then, tests are helpful to keep some sanity.
> 
> I’ll just omit this change. I figured out how to run the test and confirmed that the test doesn’t rely on files actually being there. So I don’t need to touch it at this time.

I think the project of removing all tests from LayoutTests/platform would be a relatively straightforward and quick one. If you think that should be done, lets do it soon instead of waiting.

[Alexey Proskuryakov]

I do think that this should be done, and we've sadly been waiting for years since at least a certain group of people agreed that this would be an improvement.

[Darin Adler]

Looks like there are about 300 tests in that category.

What’s less clear is exactly where we should move them. If we put them in top level platform-named directories, that seems confusing. We’ll have a directory named LayoutTests/mac and another one named LayoutTests/platform/mac, which will makes things actively worse. Let’s create a bug and discuss there what we plan to do?

[Alexey Proskuryakov]

Bug 147586 Move tests out of platform directories