Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2435273002:
Introduce spellcheck_test (Closed)
|
Created:
4 years, 2 months ago by Xiaocheng Modified:
4 years, 1 month ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce spellcheck_test
This patch introduces |spellcheck_test| for writing asynchronous layout
tests for spellchecker in an intuitive manner, following the same spirit
of |assert_selection|.
This patch allows us to convert existing spellcheck tests easily as a
preparation for idle time spellchecker.
BUG=517298
Committed: https://crrev.com/75efa395dfe1f181bd98e0a595cd3083cc3e50d7
Cr-Commit-Position: refs/heads/master@{#427035}
Patch Set 1 #Patch Set 2 : Introduce Marker class #
Total comments: 18
Patch Set 3 : spellcheck_test Mon Oct 24 14:09:33 JST 2016 #
Total comments: 6
Patch Set 4 : Address comments and disallow simultaneous running #
Total comments: 20
Patch Set 5 : Mon Oct 24 16:17:59 JST 2016 #
Total comments: 1
Patch Set 6 : Mon Oct 24 16:37:45 JST 2016 #
Total comments: 6
Patch Set 7 : Mon Oct 24 17:28:00 JST 2016 #
Messages
Total messages: 33 (17 generated)
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL. Besides, is there any tool to check if the annotations are correct?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/24 at 03:35:15, xiaochengh wrote: > PTAL. > > Besides, is there any tool to check if the annotations are correct? Please use closure compiler. It seems online version has been gone. Try below at root of chromium source: java -jar third_party\closure_compiler\compiler\compiler.jar --language_out=ES5 -W VERBOSE -O ADVANCED third_party/WebKit/LayoutTests/editing/spellcheck_test.js Use "--help" for all options for closure compiler.
Please add "!" for type annotation as much as possible you can. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html (right): https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html:9: Let's add test case for spellingMarker() and grammarMarker(). https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html:20: grammarMarker(4, 3), // 'has' Could you add a test to take array with multiple element? https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:95: if (this.description_ !== undefined) nit: We prefer early-return style to reduce indentation. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:108: if (expected.description !== undefined) { nit: We prefer early-return style. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:141: for (var i = 1; i < expectedMarkers.length; ++i) { Let's use Array#forEach. We can write: expectedMarkes.forEach((currentValue, index) => { assert_less_than(expectedMarkes[index - 1].location + ...., currentValue) }); https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:150: * @param {Node} node nit: s/Node/!Node/ It think passing |null| to |extractMarkersOfType()| should not be happened. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:174: * @param {Node} node nit: s/Node/!Node/ https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:175: * @param {Marker[]} markers s/Marker[]/!Array<!Marker>/ https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:185: * @param {Document} doc nit: s/Document/!Document/
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated. Closure compiles now reports "variable assert_foo undefined" only. PTAL. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html (right): https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html:9: On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > Let's add test case for spellingMarker() and grammarMarker(). Done. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html:20: grammarMarker(4, 3), // 'has' On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > Could you add a test to take array with multiple element? Done. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:95: if (this.description_ !== undefined) On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > nit: We prefer early-return style to reduce indentation. Done. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:108: if (expected.description !== undefined) { On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > nit: We prefer early-return style. Done. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:141: for (var i = 1; i < expectedMarkers.length; ++i) { On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > Let's use Array#forEach. We can write: > > expectedMarkes.forEach((currentValue, index) => { > assert_less_than(expectedMarkes[index - 1].location + ...., currentValue) > }); Didn't know |forEach| takes a second parameter. Done. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:150: * @param {Node} node On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > nit: s/Node/!Node/ > > It think passing |null| to |extractMarkersOfType()| should not be happened. Done. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:174: * @param {Node} node On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > nit: s/Node/!Node/ Done. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:175: * @param {Marker[]} markers On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > s/Marker[]/!Array<!Marker>/ Done. https://codereview.chromium.org/2435273002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:185: * @param {Document} doc On 2016/10/24 at 04:40:08, Yosi_UTC9 wrote: > nit: s/Document/!Document/ Done.
https://codereview.chromium.org/2435273002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:34: // { Let's use |spellingMarker()| and |grammarMarker()| in examples. Object literal can be optional or implementation details. https://codereview.chromium.org/2435273002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:144: expectedMarkers.forEach((currentMarker, index) => { How about Array#reduce() expectedMarkers.reduce((lastMarker, currentMarker) => { if (!lastMarker) return; assert_less_than(lastMarker.location + lastMarker.length, currentMarker.location); }); https://codereview.chromium.org/2435273002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:158: * @param {!Array<Marker>} markers s/!Array<Marker>/!Array<!Marker>/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I realized that we must ensure that the spellcheck tests are run sequentially, since spellchecker is affected by focus change. Added a test queue and other relevant stuff for it. PTAL. https://codereview.chromium.org/2435273002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:34: // { On 2016/10/24 at 06:17:00, Yosi_UTC9 wrote: > Let's use |spellingMarker()| and |grammarMarker()| in examples. > Object literal can be optional or implementation details. Whoops. Forgot to change this part. https://codereview.chromium.org/2435273002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:144: expectedMarkers.forEach((currentMarker, index) => { On 2016/10/24 at 06:17:00, Yosi_UTC9 wrote: > How about Array#reduce() > > expectedMarkers.reduce((lastMarker, currentMarker) => { > if (!lastMarker) > return; > assert_less_than(lastMarker.location + lastMarker.length, currentMarker.location); > }); Didn't know that. Done. https://codereview.chromium.org/2435273002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:158: * @param {!Array<Marker>} markers On 2016/10/24 at 06:17:00, Yosi_UTC9 wrote: > s/!Array<Marker>/!Array<!Marker>/ Done.
https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:61: /** @type {string|null} */ s/string|null/?string/ https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:62: this.description_ = opt_description ? opt_description : null; How about using empty string as placeholder? If so, we don't need to do null-check. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:74: /** @return {string|null} */ s/string|null/?string/ https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:86: assert_true(this.type_ === 'spelling' || this.type_ === 'grammar'); Let's have const kSpelling = 'spelling' and const kGrammar = 'grammar' to avoid spell error in code. ;-) https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:185: markers.sort((a, b) => a.location - b.location); Please avoid using one letter variable name. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:211: } catch (e) { Please avoid using one letter variable name. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:226: throw e; We prefer early-return style. e.g. if (remainingRetry <= 0) throw e; if (window.testRUnner) .... https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:275: const maxRetry = 10; s/maxRetry/kMaxRetry/ https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:277: const retryInterval = 50; s/retryInterval/kRetryInterval/ https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:290: if (testObj.properties.isSpellcheckTest !== true) Can we write |!testObj.properties.isSpellCheckTest|? Checking equality to |true| seems to be redundant.
Updated. PTAL. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:61: /** @type {string|null} */ On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > s/string|null/?string/ Done. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:62: this.description_ = opt_description ? opt_description : null; On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > How about using empty string as placeholder? If so, we don't need to do null-check. Then we need empty string check instead of null check... I prefer null since it's more different from a normal string. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:74: /** @return {string|null} */ On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > s/string|null/?string/ Done. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:86: assert_true(this.type_ === 'spelling' || this.type_ === 'grammar'); On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > Let's have const kSpelling = 'spelling' and const kGrammar = 'grammar' to avoid spell error in code. ;-) Done. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:185: markers.sort((a, b) => a.location - b.location); On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > Please avoid using one letter variable name. The comparison function is isolated out as |function markerComparison| since it's also used in |extractAllMarkers|. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:211: } catch (e) { On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > Please avoid using one letter variable name. Done. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:226: throw e; On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > We prefer early-return style. e.g. > > if (remainingRetry <= 0) > throw e; > > if (window.testRUnner) > .... Done. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:275: const maxRetry = 10; On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > s/maxRetry/kMaxRetry/ Done. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:277: const retryInterval = 50; On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > s/retryInterval/kRetryInterval/ Done. https://codereview.chromium.org/2435273002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:290: if (testObj.properties.isSpellcheckTest !== true) On 2016/10/24 at 06:58:32, Yosi_UTC9 wrote: > Can we write |!testObj.properties.isSpellCheckTest|? Checking equality to |true| seems to be redundant. Done.
https://codereview.chromium.org/2435273002/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:67: this.description_ = opt_description ? opt_description : null; Let's have const kDontCareDescription = null; or just kDontCare? Or, this.ignoreDescription_ = opt_description === undefined; this.description_ = opt_description || ''; I prefer having |ignoreDescription_|
Updated with the introduction of |ignoreDescription_|. Also added an empty-array check in |checkExpectedMarkers| since Array#reduce requires a non-empty array.
https://codereview.chromium.org/2435273002/diff/90001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:149: if (!expectedMarkers.length) Better to write: |expectedMarkers.length === 0| https://codereview.chromium.org/2435273002/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:171: for (var i = 0; i < markerCount; ++i) { nit: s/var/let/ https://codereview.chromium.org/2435273002/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:283: if (typeof(tester) === 'function') { How about moving this if-else-if fragment to |Sample| as method, e.g. executeTester() or runTest() to share with |assertSelection()|? WDYT?
Updated with making assertion failure message more user friendly. PTAL. https://codereview.chromium.org/2435273002/diff/90001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2435273002/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:149: if (!expectedMarkers.length) On 2016/10/24 at 07:54:14, Yosi_UTC9 wrote: > Better to write: |expectedMarkers.length === 0| Done. https://codereview.chromium.org/2435273002/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:171: for (var i = 0; i < markerCount; ++i) { On 2016/10/24 at 07:54:14, Yosi_UTC9 wrote: > nit: s/var/let/ Done. https://codereview.chromium.org/2435273002/diff/90001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:283: if (typeof(tester) === 'function') { On 2016/10/24 at 07:54:14, Yosi_UTC9 wrote: > How about moving this if-else-if fragment to |Sample| as method, e.g. executeTester() or runTest() to share with |assertSelection()|? > WDYT? This part is slightly different from assert_selection, as we pass a |Document| to the tester instead a |Selection|... I think passing a |Document| to tester is more convenient for spellcheck test cases. Anyway, I've added a TODO here indicating the code duplication. We may try to fix it in the future.
The CQ bit was checked by yosin@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Introduce spellcheck_test This patch introduces |spellcheck_test| for writing asynchronous layout tests for spellchecker in an intuitive manner, following the same spirit of |assert_selection|. This patch allows us to convert existing spellcheck tests easily as a preparation for idle time spellchecker. BUG=517298 ========== to ========== Introduce spellcheck_test This patch introduces |spellcheck_test| for writing asynchronous layout tests for spellchecker in an intuitive manner, following the same spirit of |assert_selection|. This patch allows us to convert existing spellcheck tests easily as a preparation for idle time spellchecker. BUG=517298 Committed: https://crrev.com/75efa395dfe1f181bd98e0a595cd3083cc3e50d7 Cr-Commit-Position: refs/heads/master@{#427035} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/75efa395dfe1f181bd98e0a595cd3083cc3e50d7 Cr-Commit-Position: refs/heads/master@{#427035} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
