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

shouldBecomeEqual() behaves as synchronous shouldBe() if the testing expression returns the expected value

First call of eval(expression) inside shouldBecomeEqual was always
made synchronously. If the testing expression returns the same value
as expected one then shouldBecomeEqual() will immediately report PASS,
for example,

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

in consequence, assuming asynchronous path of spellchecking, spelling markers
may appear after a while.

The bug was caused by checking a condition at the beginning of shouldBecomeEqual(),
before calling a timer. As a result, queued asynchronous events doesn't effect
this checking.

Since shouldBecomeEqual always checks condition asynchronously, the following
layout tests need to be synchronized a bit:
 - editing/spelling/editing-multiple-words-with-markers.html
 - editing/spelling/spellcheck-paste-continuous-disabled.html

BUG=456756
TEST=resources-tests/should-become-equal-test.html

[grzegorz, 2015-02-09 12:24:49]

g.czajkowski@samsung.com changed reviewers:
+ dpranke@chromium.org, groby@chromium.org, tony@chromium.org

[Dirk Pranke, 2015-02-09 21:02:56]

I know nothing about these tests ... +yosin, maybe?

[Dirk Pranke, 2015-02-09 21:03:06]

dpranke@chromium.org changed reviewers:
+ yosin@chromium.org

[groby-ooo-7-16, 2015-02-09 21:42:47]

groby@chromium.org changed reviewers:
+ rouslan@chromium.org

[groby-ooo-7-16, 2015-02-09 21:42:47]

Rouslan for spellcheck tests

[grzegorz, 2015-02-10 08:45:23]

Additionally, we need to add "meagesga" to MockSpellCheck::misspelled_words as a
newly added test expects it as misspelled.
https://codereview.chromium.org/907293002/

[groby-ooo-7-16, 2015-02-10 19:00:30]

groby@chromium.org changed reviewers:
- groby@chromium.org

[please use gerrit instead, 2015-02-10 22:32:52]

Since you're modifying the original tests from
http://trac.webkit.org/changeset/168426 so much, you don't need to stick with
"meagesga" as the misspelled word. You can use any other word already used in
layout tests. For example, type "it's a Curabitur Curabitur ", then delete
"bitur Cura".

[please use gerrit instead, 2015-02-10 22:55:42]

Thank you for the patch! Adding a failing test and immediately disabling it is
dangerous, because we don't really know the cause of the failure. There are two
better alternatives, in order of preference:

1) Fix the code such that the test is passing.
2) Set the expectations to the output of the failing test.

I have a few comments inline, but in general I would prefer if this test looked
more like the other spellcheck tests, since you're already modifying the test
from WebKit.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/editing-multiple-words-with-markers.html
(right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:27:
typeText(textArea, "it's a meagesga meagesga ");
This test will be flaky if you don't wait for spellcheck markers to appear. Use
internals.markerCountForNode(). See resources/utils.js for example.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:31: *
Those test cases verify if spelling markes disappear when
s/markes/markers/

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:69: return
done();
If window.internals is not defined, this statement will loop through the
testCases without doing any work. Why not check for window.internals before
calling done(). That way you can avoid looping through testCases if no tests can
be performed.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:71:
debug(testElement.value);
Is debugging necessary here?

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:78:
debug("");
Is debugging necessary here?

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:86: if
(nextTestCase)
I would feel better if you put curly braces around your multiline if statement
body.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:87: return
setTimeout(checkSpellingMarkerAfterDeleteingSelection, 0,
Why setTimeout(func, 0, args) instead of simply func(args)?

[please use gerrit instead, 2015-02-10 22:57:50]

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/spellcheck-paste-continuous-disabled.html
(right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/spellcheck-paste-continuous-disabled.html:30:
shouldBecomeEqual('internals.hasSpellingMarker(document, 0, 2)', 'true',
function() {
If this fixes flakiness, then you should separate this change into a separate
patch.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/spellcheck-paste-disabled.html (right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/spellcheck-paste-disabled.html:31:
shouldBecomeEqual('internals.hasSpellingMarker(document, 0, 2)', 'true',
function(){
Ditto.

[please use gerrit instead, 2015-02-10 22:59:20]

https://codereview.chromium.org/906193003/diff/1/LayoutTests/resources/js-tes...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/resources/js-tes...
LayoutTests/resources/js-test.js:316: setTimeout(_waitForCondition, 5,
_condition, _failureTime, _completionHandler, _failureHandler);
This is not necessary, because _waitForCondition() already runs setTimeout
internally.

[grzegorz, 2015-02-11 10:54:23]

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/editing-multiple-words-with-markers.html
(right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:27:
typeText(textArea, "it's a meagesga meagesga ");
On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> This test will be flaky if you don't wait for spellcheck markers to appear.
Use
> internals.markerCountForNode(). See resources/utils.js for example.

Done.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:31: *
Those test cases verify if spelling markes disappear when
On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> s/markes/markers/

Done.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:69: return
done();
On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> If window.internals is not defined, this statement will loop through the
> testCases without doing any work. Why not check for window.internals before
> calling done(). That way you can avoid looping through testCases if no tests
can
> be performed.

Done.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:71:
debug(testElement.value);
On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> Is debugging necessary here?

It adds empty line in the output.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:78:
debug("");
On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> Is debugging necessary here?

Ditto.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:86: if
(nextTestCase)
On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> I would feel better if you put curly braces around your multiline if statement
> body.

Done.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:87: return
setTimeout(checkSpellingMarkerAfterDeleteingSelection, 0,
On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> Why setTimeout(func, 0, args) instead of simply func(args)?

I can change it if you like.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/spellcheck-paste-continuous-disabled.html
(right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/spellcheck-paste-continuous-disabled.html:30:
shouldBecomeEqual('internals.hasSpellingMarker(document, 0, 2)', 'true',
function() {
On 2015/02/10 22:57:50, Rouslan Solomakhin wrote:
> If this fixes flakiness, then you should separate this change into a separate
> patch.

Since shouldBecomeEqual() now works asynchronously, we have to call the next
checking in its completion handler. Otherwise the test will finish earlier than
result of second shouldBecmeEqual() request.

It's strongly related to this CL.

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/spellcheck-paste-disabled.html (right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/spellcheck-paste-disabled.html:31:
shouldBecomeEqual('internals.hasSpellingMarker(document, 0, 2)', 'true',
function(){
On 2015/02/10 22:57:50, Rouslan Solomakhin wrote:
> Ditto.

Ditto :)

https://codereview.chromium.org/906193003/diff/1/LayoutTests/resources/js-tes...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/resources/js-tes...
LayoutTests/resources/js-test.js:316: setTimeout(_waitForCondition, 5,
_condition, _failureTime, _completionHandler, _failureHandler);
On 2015/02/10 22:59:20, Rouslan Solomakhin wrote:
> This is not necessary, because _waitForCondition() already runs setTimeout
> internally.

Not really, assuming existing implementation, before _waitForCondition() we
check condition above and remember it in '_condition' variable, and then inside
_waitForCondition() we check it. For example, 

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

is always passing as hasSpellingMarker() returns false as the spelling markers
were requested and they will be available later. Actually, there is no
difference between

shouldBecomeEqual(internals.hasSpellingMarker(document, "welllcome"), false);
and
shouldBecomeEqual(internals.hasSpellingMarker(document, "welcome"), false);

which makes shouldBecomeEqual unreliable.

I tried to describe this in the commit-msg.

[grzegorz, 2015-02-11 11:53:28]

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
File LayoutTests/editing/spelling/editing-multiple-words-with-markers.html
(right):

https://codereview.chromium.org/906193003/diff/1/LayoutTests/editing/spelling...
LayoutTests/editing/spelling/editing-multiple-words-with-markers.html:27:
typeText(textArea, "it's a meagesga meagesga ");
On 2015/02/11 10:54:23, grzegorz wrote:
> On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> > This test will be flaky if you don't wait for spellcheck markers to appear.
> Use
> > internals.markerCountForNode(). See resources/utils.js for example.
> 
> Done.

The problem here is a little complicated. Waiting for spelling markers here is
fine if we want to simulate a typical user behaviour. Usually it looks like:
1. User types: "it's a meagesga meagesga "
2. Spelling markers appear
3. User selects a range
4. User deletes a range.
5. Spelling marker disappear (caret in the middle of the word) 

However, if we do it through the script the markers from typing are applied
AFTER selection is removed. See
https://code.google.com/p/chromium/issues/detail?id=456756 for more details.
There is a simple test case as well.

My goal here is to show that shouldBecomeEqual has a bug and to prove that I
added a spelling test which contains test cases where spelling markers are not
expected. Assuming current implementation of shouldBecomeEqual this test passes.

However, the bug (https://code.google.com/p/chromium/issues/detail?id=456756)
refers to

[grzegorz, 2015-02-11 12:07:15]

On 2015/02/10 22:55:42, Rouslan Solomakhin wrote:
> Thank you for the patch! Adding a failing test and immediately disabling it is
> dangerous, because we don't really know the cause of the failure. There are
two
> better alternatives, in order of preference:
> 
> 1) Fix the code such that the test is passing.

Since it happens in the scripts only I would like to know what do you guys
thinks about it? Would you like to have a fix or it's acceptable as it is
(https://code.google.com/p/chromium/issues/detail?id=456756)

> 2) Set the expectations to the output of the failing test.
> 

I think that the test should go along with the fix in spellchecker code.
However, will you post l_g_t_m for changes in shouldBecomeEqual() withou the
test?

[please use gerrit instead, 2015-02-11 21:53:38]

I would approve changes to shouldBecomeEqual that fix a problem with
shouldBecomeEqual, only if you also have a test case for the fix. That is, add a
test case for shouldBecomeEqual that fails without a fix, but succeeds with your
fix. The test should probably be located in something like
LayoutTests/resources/should-become-equal-test.js and have expectations in
something like LayoutTests/resources/should-become-equal-test-expected.txt. Can
you please write such a patch? Thank you. (You can file a separate bug for this,
if you want.) Ping me if you have questions.

[grzegorz, 2015-02-12 13:38:30]

On 2015/02/11 21:53:38, Rouslan Solomakhin wrote:
> I would approve changes to shouldBecomeEqual that fix a problem with
> shouldBecomeEqual, only if you also have a test case for the fix. That is, add
a
> test case for shouldBecomeEqual that fails without a fix, but succeeds with
your
> fix. The test should probably be located in something like
> LayoutTests/resources/should-become-equal-test.js and have expectations in
> something like LayoutTests/resources/should-become-equal-test-expected.txt.
Can
> you please write such a patch? Thank you. (You can file a separate bug for
this,
> if you want.) Ping me if you have questions.

Thanks. I will add a regression test.

[grzegorz, 2015-02-17 08:39:37]

Any comments on this?

[please use gerrit instead, 2015-02-17 16:28:48]

What do Blink people think? I'm concerned that this change slows down by 5
seconds the tests that use shouldBecomeEqual(). I don't think that's a good idea
and actually will make some tests more flaky, because more than 5  seconds wait
will be required on a slow or heavily loaded machine.

grzegorz@: Sorry I missed this patch. We usually start a new patch if the
purpose of the patch changes entirely.

https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources-te...
File LayoutTests/resources-tests/should-become-equal-test.html (right):

https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources-te...
LayoutTests/resources-tests/should-become-equal-test.html:28:
shouldBecomeEqual("internals.hasSpellingMarker(document, 0, 8)", "false",
function() {
You probably should not be testing shouldBecomeEqual() using the spellcheck
code. Let's not make the utility code be dependent on spellcheck code. It should
be other way around. Please define some other function in this file that can be
tested using shouldBecomeEqual().

https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources/js...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources/js...
LayoutTests/resources/js-test.js:316: setTimeout(_waitForCondition, 5,
_condition, _failureTime, _completionHandler, _failureHandler);
I'm still quite queasy about this change. We are slowing down each one of our
tests by 5 seconds? That's bad.

[Dirk Pranke, 2015-02-17 22:09:49]

I apologize, in that I already punted out of this CL once by disclaiming any
knowledge of this, so I hate to
do so twice, but hard-coding a 5 second timeout for anything is not okay. Very
few tests should need to take that long to run, and hard coding any single value
is just going to lead to flakiness.

I haven't looked at the details of this CL. Should I?

[please use gerrit instead, 2015-02-17 22:11:26]

I would agree that we need to discard this approach. If there's a problem, we
should take a different approach.

[grzegorz, 2015-02-18 15:32:00]

On 2015/02/17 16:28:48, Rouslan Solomakhin wrote:
> What do Blink people think? I'm concerned that this change slows down by 5
> seconds the tests that use shouldBecomeEqual(). I don't think that's a good
idea
> and actually will make some tests more flaky, because more than 5  seconds
wait
> will be required on a slow or heavily loaded machine.
> 

5 seconds? window.setTimeout() takes this number in milliseconds :) There is no
flaky tests which you mention. BTW we already use this value as interval when we
waiting for result in shouldBecomeEqual(). What's we could even change this
value to 0. The reason for it is to give queued synchronous events chance to
appear which I describe in cmg-msg (hopefully well).

> grzegorz@: Sorry I missed this patch. We usually start a new patch if the
> purpose of the patch changes entirely.

Ok

> 
>
https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources-te...
> File LayoutTests/resources-tests/should-become-equal-test.html (right):
> 
>
https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources-te...
> LayoutTests/resources-tests/should-become-equal-test.html:28:
> shouldBecomeEqual("internals.hasSpellingMarker(document, 0, 8)", "false",
> function() {
> You probably should not be testing shouldBecomeEqual() using the spellcheck
> code. Let's not make the utility code be dependent on spellcheck code. It
should
> be other way around. Please define some other function in this file that can
be
> tested using shouldBecomeEqual().

I could think about it if the general idea is fine for you. Would appreciate any
example.

> 
>
https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources/js...
> File LayoutTests/resources/js-test.js (right):
> 
>
https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources/js...
> LayoutTests/resources/js-test.js:316: setTimeout(_waitForCondition, 5,
> _condition, _failureTime, _completionHandler, _failureHandler);
> I'm still quite queasy about this change. We are slowing down each one of our
> tests by 5 seconds? That's bad.

It's not true. It's 5 millisecond.

[please use gerrit instead, 2015-02-18 15:57:18]

Ah, 5 ms. Sorry I misread your code. That's not as bad. Still, I would prefer
that we don't make this change. Instead we should do the necessary waits inside
of the test for execDeleteCommand(). Do you think it would be better to discuss
how to fix execDeleteCommand() on blink-dev@ first?

[yosin_UTC9, 2015-02-19 04:53:20]

On 2015/02/18 15:57:18, Rouslan Solomakhin wrote:
> Ah, 5 ms. Sorry I misread your code. That's not as bad. Still, I would prefer
> that we don't make this change. Instead we should do the necessary waits
inside
> of the test for execDeleteCommand(). Do you think it would be better to
discuss
> how to fix execDeleteCommand() on blink-dev@ first?

How about make |internals.hasSpellingMarker()| returns |Promise| rather than
wait something with introducing |ShouldPromiseEqual()| and
|ShouldPromiseDifferent|?

The sketch of |ShouldPromiseEqual(expr, expected)| is

function ShouldPromiseEqual(expr, expected) {
  try {
    let promise = eval(expr);
    promise.then((actual) => { 
      if (actual === expected) {
       Passed(expr, actual);
       return;
      }
      Failed(expr, expected, actual);
    }).catch((reason) => { ... report exception in expr });    
  } catch (e) {
    ... report exception in |eval(expr)|.
  }
}

[grzegorz, 2015-02-19 15:31:02]

On 2015/02/19 04:53:20, Yosi_UTC9 wrote:
> On 2015/02/18 15:57:18, Rouslan Solomakhin wrote:
> > Ah, 5 ms. Sorry I misread your code. That's not as bad. Still, I would
prefer
> > that we don't make this change. Instead we should do the necessary waits
> inside
> > of the test for execDeleteCommand(). Do you think it would be better to
> discuss
> > how to fix execDeleteCommand() on blink-dev@ first?
> 
> How about make |internals.hasSpellingMarker()| returns |Promise| rather than
> wait something with introducing |ShouldPromiseEqual()| and
> |ShouldPromiseDifferent|?
> 
> The sketch of |ShouldPromiseEqual(expr, expected)| is
> 
> function ShouldPromiseEqual(expr, expected) {
>   try {
>     let promise = eval(expr);
>     promise.then((actual) => { 
>       if (actual === expected) {
>        Passed(expr, actual);
>        return;
>       }
>       Failed(expr, expected, actual);
>     }).catch((reason) => { ... report exception in expr });    
>   } catch (e) {
>     ... report exception in |eval(expr)|.
>   }
> }

Looks promising for me. I am not much familiar with promises but I wonder how
all those Internals:: spelling verification methods will look as now they simply
check whether there is an appropriate marker or not. As for testing side, we do
not have any callbacks to notify us whether spelling marker appear or not.

Do you mean that SpellCheckerRequster::didCheckSucceed() should notify text to
be marked as well as Internals object which could listen on those event and call
resolved/rejected ? Without those event we'll end up waiting as well. I probably
miss something. Would appreciate any sketch of |hasSpellingMaker| returning
|Promise|.

Thank you.

[please use gerrit instead, 2015-02-19 15:38:46]

> As for testing side, we do
> not have any callbacks to notify us whether spelling marker appear or not.

FYI, the current way to test spelling markers is to wait for them to
appear/disappear with a timeout. See this example of how this works currently:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[please use gerrit instead, 2015-04-23 16:36:37]

rouslan@chromium.org changed reviewers:
- rouslan@chromium.org

[Dirk Pranke, 2016-08-31 19:01:46]

dpranke@chromium.org changed reviewers:
- dpranke@chromium.org

[yosin_UTC9, 2016-09-01 01:49:07]

https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources-te...
File LayoutTests/resources-tests/should-become-equal-test.html (right):

https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources-te...
LayoutTests/resources-tests/should-become-equal-test.html:1: <html>
Could you use w3c test harness to test |shouldBecomeEqual()|?
https://github.com/w3c/testharness.js/blob/master/docs/api.md

BTW, ideal solution is using |asyn_test()| in w3c test harness.
At this time there are 70 files using |shouldBecomeEqual()|,
it is too far away to reach ideal world. :-<

https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources/js...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/906193003/diff/20001/LayoutTests/resources/js...
LayoutTests/resources/js-test.js:316: setTimeout(_waitForCondition, 5,
_condition, _failureTime, _completionHandler, _failureHandler);
On 2015/02/17 at 16:28:48, rouslan wrote:
> I'm still quite queasy about this change. We are slowing down each one of our
tests by 5 seconds? That's bad.

It is "5ms" instead of "5s".
https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers/setTimeout

Could you have |const| variable to describe unit of delay?
e.g. const kDelayMilliseconds = 5;

BTW, 5ms is too small, since frame rate is 60Hz. 16ms may be enough.