Bug 133939 – shouldBecomeEqual() behaves as shouldBe() if the testing expression returns the expected value

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

[Grzegorz Czajkowski]

Once the tested expression returns different value than expected one, shouldBecomeEqual() properly waits until the expression becomes the expected one.

In case when the tested expression returns the same value as expected one, shouldBecomeEqual() reports PASS. However, there are some cases when we should wait weather the tested expression remains the expected value. Let me give an example:

shouldBecomeEqual("internals.hasSpellingMarker(...)", "false");

is always reporting PASS which is not true (assuming asynchronous path of spellchecking is turned on). The spelling marker may appear after a while.

The reason for that is we are checking a condition at the beginning of shouldBecomeEqual(), before calling a timer. As a result, queued asynchronous events doesn't effect on this checking.

How should we handle this? If I we moved checking values to the timer shouldBecomeEqual would be more reliable IMHO. However, one timer execution does not guarantee that the expression will remain the tested value. We could consider checking at few attempts or adding an additional timer.

What's your opinion on that? Thanks

[Grzegorz Czajkowski]

Created attachment 239473 [details]
Patch

[Build Bot]

Comment on attachment 239473 [details]
Patch

Attachment 239473 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4947001070321664

New failing tests:
editing/spelling/grammar-edit-word.html

[Build Bot]

Created attachment 239477 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Grzegorz Czajkowski]

(In reply to comment #2)
> (From update of attachment 239473 [details])
> Attachment 239473 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/4947001070321664
> 
> New failing tests:
> editing/spelling/grammar-edit-word.html

After Ryosuke asynchronous selection changes (r164401), updateEditorUINowIfScheduled() needs to be called to notify spellchecker about changes.

I need to confirm this using Mac as it only supports grammar checking and I'll take care of this in the next round.

[Darin Adler]

Comment on attachment 239473 [details]
Patch

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

> LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:72
> +        setTimeout(execDeleteCommand, 100);

What determined the magic number 100 here?

> LayoutTests/resources/js-test-pre.js:310
> +  setTimeout(_waitForCondition, 5, condition, completionHandler);

What determined the magic number 5 here?

> LayoutTests/resources/js-test-pre.js:422
> +  setTimeout(_waitForCondition, 5, condition, completionHandler);

What determined the magic number 5 here?

[Grzegorz Czajkowski]

Comment on attachment 239473 [details]
Patch

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

Thanks for the review!

>> LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:72
>> +        setTimeout(execDeleteCommand, 100);
> 
> What determined the magic number 100 here?

Actually, I stole this value from http://trac.webkit.org/changeset/164401/trunk/LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js. I need to experiment with it a little to be less magic and get the test to pass.

>> LayoutTests/resources/js-test-pre.js:310
>> +  setTimeout(_waitForCondition, 5, condition, completionHandler);
> 
> What determined the magic number 5 here?

It is being used by _waitForCondition() inside. I don't mind changing it to 0 here.

>> LayoutTests/resources/js-test-pre.js:422
>> +  setTimeout(_waitForCondition, 5, condition, completionHandler);
> 
> What determined the magic number 5 here?

Ditto.

[Grzegorz Czajkowski]

Created attachment 239725 [details]
Take Darin's comments into account and fix grammar-edit-word.html

[Build Bot]

Comment on attachment 239725 [details]
Take Darin's comments into account and fix grammar-edit-word.html

Attachment 239725 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5643287612358656

New failing tests:
editing/spelling/grammar-edit-word.html

[Build Bot]

Created attachment 239727 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Grzegorz Czajkowski]

Created attachment 243435 [details]
Fix for grammar-edit-word.html

[Darin Adler]

Comment on attachment 243435 [details]
Fix for grammar-edit-word.html

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

I’ll say review+ but I do still have some concerns.

> Source/WebCore/testing/Internals.cpp:1564
>          return 0;

Should be false, not 0.

> Source/WebCore/testing/Internals.cpp:1566
> +    updateEditorUINowIfScheduled();

Is this the only function in this file that is missing this? Can we do a quick audit to look for others?

> LayoutTests/ChangeLog:30
> +        Replace magic number 100 with newly defined value.

But what makes the new magic number OK?

> LayoutTests/ChangeLog:39
> +        However, timeout needs to be added as while grammar checking
> +        Editor::respondToChangedSelection() is invoked in some circumstances
> +        and causes asynchronous spellchecker updates.

That sounds really vague. Is there some way we can pin this down and fix it some other way besides adding a timeout-based polling loop?

> LayoutTests/ChangeLog:49
> +        * resources/js-test-pre.js:
> +        (shouldBecomeEqual):
> +        (shouldBecomeDifferent):
> +        Always check a condition on timer.

There is another copy of these functions in js-test.js. It’s not great to change one file and not the other.

> LayoutTests/editing/spelling/resources/util.js:1
> +const timeoutToApplyPendingProgramaticSelectionChanges = 50;

What guarantees that we use this timeout only when running tests in browser? We don’t want timeouts with magic values like this in the run-webkit-tests version, because that’s inherently going to lead to racy and flaky tests when they run on machines with different speeds and loads.

[Grzegorz Czajkowski]

(In reply to comment #11)
> Comment on attachment 243435 [details]
> Fix for grammar-edit-word.html
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243435&action=review
> 
> I’ll say review+ but I do still have some concerns.
> 
> > Source/WebCore/testing/Internals.cpp:1564
> >          return 0;
> 
> Should be false, not 0.

Definitely, submitted patch separately (bug 139766) because it's not strictly related to this change.

> 
> > Source/WebCore/testing/Internals.cpp:1566
> > +    updateEditorUINowIfScheduled();
> 
> Is this the only function in this file that is missing this? Can we do a
> quick audit to look for others?

Yes, it's only one method where we missed it.

> 
> > LayoutTests/ChangeLog:30
> > +        Replace magic number 100 with newly defined value.
> 
> But what makes the new magic number OK?

Unfortunately, it seems that we still need a timeout (non zero) to properly handle markers although we earlier called internals.updateEditorUINowIfScheduled() which should synchronously update them. Neither I am doing something wrong or we are having bug somewhere.

> 
> > LayoutTests/ChangeLog:39
> > +        However, timeout needs to be added as while grammar checking
> > +        Editor::respondToChangedSelection() is invoked in some circumstances
> > +        and causes asynchronous spellchecker updates.
> 
> That sounds really vague. Is there some way we can pin this down and fix it
> some other way besides adding a timeout-based polling loop?

I agree, I'll investigate it.

> 
> > LayoutTests/ChangeLog:49
> > +        * resources/js-test-pre.js:
> > +        (shouldBecomeEqual):
> > +        (shouldBecomeDifferent):
> > +        Always check a condition on timer.
> 
> There is another copy of these functions in js-test.js. It’s not great to
> change one file and not the other.

I was not aware of their duplication. In the meantime I submitted patch which makes no difference between them (bug 139767).

> 
> > LayoutTests/editing/spelling/resources/util.js:1
> > +const timeoutToApplyPendingProgramaticSelectionChanges = 50;
> 
> What guarantees that we use this timeout only when running tests in browser?
> We don’t want timeouts with magic values like this in the run-webkit-tests
> version, because that’s inherently going to lead to racy and flaky tests
> when they run on machines with different speeds and loads.

That's my concern as well, my goal is to:

    AnySelectionChange;
    if (internals) {
        internals.updateEditorUINowIfScheduled();
        DoAnotherSelectionChange;
     } else {
        setTiemout(DoAnotherSelectionChange, 0);
        ...
     }
but never both. Unfortunately, it seems that it's not possible now. Need some time to have another look at this.

[Grzegorz Czajkowski]

(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 243435 [details]
> > Fix for grammar-edit-word.html
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=243435&action=review
> > 
> > > LayoutTests/ChangeLog:39
> > > +        However, timeout needs to be added as while grammar checking
> > > +        Editor::respondToChangedSelection() is invoked in some circumstances
> > > +        and causes asynchronous spellchecker updates.
> > 
> > That sounds really vague. Is there some way we can pin this down and fix it
> > some other way besides adding a timeout-based polling loop?
> 
> I agree, I'll investigate it.
> 

The root cause of tests failure [1] is the same as both run execCommand("Delete").

In one task in TypingCommand::deleteKeyPressed we are doing two selection changes:

1. deleteSelection(selectionToDelete, m_smartDelete);
   calls respondToChangedSelection() and editorUIUpdateTimer.startOneShot(0)

2. typingAddedToOpenCommand(DeleteKey); 
   calls respondToChangedSelection() but editorUIUpdateTimer is still active and we do an early return and do not notify spellchecker about this selection change.

Invoking updateEditorUINowIfScheduled() does not help after applying command as we already lost one selection change.

I am going to create a separate bug for this and mark them as failure. 


[1] grammar-edit-word.html, editing-multiple-words-with-markers.html

[Grzegorz Czajkowski]

Created attachment 243623 [details]
Patch

[Grzegorz Czajkowski]

(In reply to comment #14)
> Created attachment 243623 [details]
> Patch

Feel free to reject commit request if you prefer fixing those issues in the same bug but it may take some time.

[WebKit Commit Bot]

Comment on attachment 243623 [details]
Patch

Clearing flags on attachment: 243623

Committed r177682: <http://trac.webkit.org/changeset/177682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.