DevTools: Add test for SourceCodeDiff (issue 2381953004 by einbinder@chromium.org)

0 views
Skip to first unread message

einb...@chromium.org

unread,
Sep 29, 2016, 3:48:08 PM9/29/16
to lush...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org
Reviewers: lushnikov
CL: https://codereview.chromium.org/2381953004/

Message:
ptal

Description:
DevTools: Add test for SourceCodeDiff

BUG=none

Affected files (+238, -0 lines):
A third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css
A third_party/WebKit/LayoutTests/inspector/sources/resources/diff-before.css
A third_party/WebKit/LayoutTests/inspector/sources/source-code-diff.html
A third_party/WebKit/LayoutTests/inspector/sources/source-code-diff-expected.txt
M third_party/WebKit/Source/devtools/front_end/sources/SourceCodeDiff.js


lush...@chromium.org

unread,
Sep 29, 2016, 5:10:34 PM9/29/16
to einb...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org
shell we add a bunch of these tests? You've spotted a few corner cases with
specific editions of first lines - should we cover those?


https://codereview.chromium.org/2381953004/diff/1/third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css
File
third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css
(right):

https://codereview.chromium.org/2381953004/diff/1/third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css#newcode3
third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css:3:
width: 100%;
i failed to diff this against the other version manually. Imaging test
start failing - it would be hard to understand what's going on

Let's come up with something small and human-verifiable

https://codereview.chromium.org/2381953004/diff/1/third_party/WebKit/LayoutTests/inspector/sources/source-code-diff-expected.txt
File
third_party/WebKit/LayoutTests/inspector/sources/source-code-diff-expected.txt
(right):

https://codereview.chromium.org/2381953004/diff/1/third_party/WebKit/LayoutTests/inspector/sources/source-code-diff-expected.txt#newcode3
third_party/WebKit/LayoutTests/inspector/sources/source-code-diff-expected.txt:3:
0:@
I initially thought that "@" is "not changed/context" until I opened the
diff-after.css.

Now i think it's "modified". Shell we use "modified/added/removed" to
avoid confusion?

https://codereview.chromium.org/2381953004/

einb...@chromium.org

unread,
Sep 29, 2016, 6:33:18 PM9/29/16
to lush...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org
This test is going to get much more complicated and cover more edge cases once
you can revert changes.



https://codereview.chromium.org/2381953004/diff/1/third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css
File
third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css
(right):

https://codereview.chromium.org/2381953004/diff/1/third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css#newcode3
third_party/WebKit/LayoutTests/inspector/sources/resources/diff-after.css:3:
width: 100%;
On 2016/09/29 at 21:10:33, lushnikov wrote:
> i failed to diff this against the other version manually. Imaging test
start failing - it would be hard to understand what's going on
>
> Let's come up with something small and human-verifiable

Done.
On 2016/09/29 at 21:10:33, lushnikov wrote:
> I initially thought that "@" is "not changed/context" until I opened
the diff-after.css.
>
> Now i think it's "modified". Shell we use "modified/added/removed" to
avoid confusion?

lush...@chromium.org

unread,
Sep 30, 2016, 12:29:45 PM9/30/16
to einb...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org
lgtm

It would be also nice to have a test that triggers that "merge" behavior you've
seen in the code

https://codereview.chromium.org/2381953004/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 6:14:34 PM9/30/16
to einb...@chromium.org, lush...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 10:12:03 PM9/30/16
to einb...@chromium.org, lush...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org
Committed patchset #2 (id:20001)

https://codereview.chromium.org/2381953004/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 10:13:49 PM9/30/16
to einb...@chromium.org, lush...@chromium.org, commi...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, chromium...@chromium.org, devtools...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, pfel...@chromium.org
Patchset 2 (id:??) landed as
https://crrev.com/59102e2afaceec67dcab87bca2e5a2505f3202a3
Cr-Commit-Position: refs/heads/master@{#422283}

https://codereview.chromium.org/2381953004/
Reply all
Reply to author
Forward
0 new messages