Make NGBlockLayoutAlgorithmTest inherit RenderingTest

Make NGBlockLayoutAlgorithmTest inherit RenderingTest

With RenderingTest it's possible to separate HTML test representation and unittest validation logic.

BUG=635619
Review-Url: https://codereview.chromium.org/2560233003
Cr-Commit-Position: refs/heads/master@{#444988}
Committed: https://chromium.googlesource.com/chromium/src/+/ff9ccb9534870244065db87766a709a23647ff6f

[Gleb Lanbin, 2016-12-09 23:14:58]

glebl@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[Gleb Lanbin, 2016-12-09 23:16:04]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 23:17:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gleb Lanbin, 2016-12-09 23:18:17]

Description was changed from

==========
Convert NGBlockLayoutAlgorithmTest.CollapsingMarginsCase1 to a SimTest.

The main benefit of SimTest is that it eliminates the necessity to construct a
test case HTML representation manually and makes it read directly from file.

BUG=635619
==========

to

==========
Convert NGBlockLayoutAlgorithmTest.CollapsingMarginsCase1 to a SimTest.

The main benefit of SimTest is that it eliminates the necessity to construct a
test case HTML representation manually and makes it read directly from file.

BUG=635619
==========

[Gleb Lanbin, 2016-12-09 23:18:17]

glebl@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[commit-bot: I haz the power, 2016-12-10 01:19:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 01:19:33]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Gleb Lanbin, 2016-12-10 01:22:01]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 01:23:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 04:00:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 04:00:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Gleb Lanbin, 2016-12-12 21:24:50]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2016-12-12 21:24:57]

Patchset #1 (id:20001) has been deleted

[Gleb Lanbin, 2016-12-12 21:30:35]

Description was changed from

==========
Convert NGBlockLayoutAlgorithmTest.CollapsingMarginsCase1 to a SimTest.

The main benefit of SimTest is that it eliminates the necessity to construct a
test case HTML representation manually and makes it read directly from file.

BUG=635619
==========

to

==========
Make NGBlockLayoutAlgorithmTest inherit RenderingTest

With RenderingTest it's possible to separate HTML test representation and
unittest validation logic.

BUG=635619
==========

[Gleb Lanbin, 2016-12-12 21:30:48]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 21:31:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gleb Lanbin, 2016-12-12 21:40:57]

glebl@chromium.org changed reviewers:
+ eae@chromium.org

[Gleb Lanbin, 2016-12-12 21:40:57]

adding eae@ for third_party/WebKit/Source/platform

[cbiesinger, 2016-12-12 22:13:23]

The main downside of a simtest is that you're not really testing layoutng, you
are testing the combination of layoutng with the legacy integration. Do you
think that's worth it?

[Gleb Lanbin, 2016-12-12 23:07:29]

On 2016/12/12 22:13:23, cbiesinger wrote:
> The main downside of a simtest is that you're not really testing layoutng, you
> are testing the combination of layoutng with the legacy integration. Do you
> think that's worth it?

update the patch with an examples that tests LayoutNG code but still uses
injected HTML layout tree.
From my personal experience creating LayoutTree manually is a bit error prone.
Also I think we do want to test how LayoutNG interacts with legacy code (e.g. we
have "Render google.com in LayoutNG" OKR).

[eae, 2017-01-03 04:47:47]

Like Christian said this tests things slightly differently. That's perfectly
fine but something to be aware of. Using this still of tests in conjunction with
regular unit tests sounds like a good idea to me. It is no replacement however.

Does that make sense to both of you?

If so, LGTM w/suggestions.

https://codereview.chromium.org/2560233003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/test_data/margins_collapse1.html
(right):

https://codereview.chromium.org/2560233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/test_data/margins_collapse1.html:1:
<!DOCTYPE html>
Please add a comment explaining how this html file is used and include a pointer
to the unit test.

[cbiesinger, 2017-01-03 15:30:59]

Yes that sounds good to me.

[Gleb Lanbin, 2017-01-19 22:30:40]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 22:31:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 22:36:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 22:36:38]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Gleb Lanbin, 2017-01-20 00:24:04]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 00:25:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 02:43:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 02:43:06]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-01-20 05:43:51]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-01-20 05:43:52]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2560233003/#ps100001
(title: "revert ScopedRuntimeEnabledFeatureForTest")

[commit-bot: I haz the power, 2017-01-20 05:44:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 05:48:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484891031822050,
"parent_rev": "8729d0519f3f26c833e6ece958e3fd3c16d1e2b5", "commit_rev":
"ff9ccb9534870244065db87766a709a23647ff6f"}

[commit-bot: I haz the power, 2017-01-20 05:49:46]

Description was changed from

==========
Make NGBlockLayoutAlgorithmTest inherit RenderingTest

With RenderingTest it's possible to separate HTML test representation and
unittest validation logic.

BUG=635619
==========

to

==========
Make NGBlockLayoutAlgorithmTest inherit RenderingTest

With RenderingTest it's possible to separate HTML test representation and
unittest validation logic.

BUG=635619

Review-Url: https://codereview.chromium.org/2560233003
Cr-Commit-Position: refs/heads/master@{#444988}
Committed:
https://chromium.googlesource.com/chromium/src/+/ff9ccb9534870244065db87766a7...
==========

[commit-bot: I haz the power, 2017-01-20 05:49:47]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ff9ccb9534870244065db87766a7...