New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 604060 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 522654

Blocking:
issue 354261



Sign in to add a comment

Some unit tests in gyp build are not in the gn build

Project Member Reported by h...@chromium.org, Apr 15 2016

Issue description

(and potentially vice versa?)

For example, in the gyp build, view_targeter_unittest.cc is part of views_unittests, but not so in the gn build.
 
Cc: brettw@chromium.org brucedaw...@chromium.org
Components: Build
Labels: -Pri-3 Pri-2
Owner: brucedaw...@chromium.org
Status: Assigned (was: Available)
Comparing a gyp release build with:
    GYP_DEFINES=component=shared_library disable_nacl=1 win_fastlink=1
to a gn build with:
    is_debug = false
    is_component_build = true
    enable_nacl = false
    is_win_fastlink = true
at hash d570b1f16e30edb5b6b7254c9549d1355fe1cd03 I see 27397 invocations of cl.exe in the gyp build and only 26929 in the gn build, a 468 translation unit discrepancy.

This is probably a mixture of build target differences and omitted source files. I'm planning to scrape the data to find where the differences are coming from.

Getting the differences to zero is probably a non-goal, but understanding them seems useful. If you have any information on known differences (benign or otherwise) please share.

You're certainly not going to be able to get to zero.

Also, checking w/ enable_nacl=false is misleading, since you'll be missing a lot of stuff.

That said, I have no doubt that we may be missing things.
I knew that I had to set gn to x86 to make them match but then I forgot to actually do it. Now I'm testing with that fixed and with enable_nacl=true. So:
    GYP_DEFINES=component=shared_library win_fastlink=1
and:
    is_debug = false
    is_component_build = true
    enable_nacl = true
    is_win_fastlink = true
    target_cpu="x86"

Comment 5 by brettw@chromium.org, Apr 30 2016

I wouldn't count compile steps, those are definitely going to be different.

When we flipped Linux, I compiled the unit tests binaries, did --gtest-list-tests (or something like that, sorted the result, then diffed.

This worked well.
Cc: scottmg@chromium.org
Didn't Scott build something to compare diffs of lists of compilation units in gyp and gn a while ago that found some of this?
The paths for generated files are certainly different, but is there reasons to expect the set of checked-in .cc files being built to be different?

A quick comparison shows that there appear to be 30 *test?.cc files that are compiled in gyp builds but not gn builds.

Another source of diffs is files that are platform specific that have been filtered on gn builds but not gyp builds. These are good differences - more reasons to look forward to dropping gyp.
Project Member

Comment 8 by bugdroid1@chromium.org, May 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/715326a623d42be037eca4c333d9e746eca03ddb

commit 715326a623d42be037eca4c333d9e746eca03ddb
Author: brucedawson <brucedawson@chromium.org>
Date: Wed May 04 20:47:20 2016

Add missing remoting tests to gn build

Some .cc files are missing from the .gn build. Adding a few of them.

BUG= 604060 

Review-Url: https://codereview.chromium.org/1946373002
Cr-Commit-Position: refs/heads/master@{#391631}

[modify] https://crrev.com/715326a623d42be037eca4c333d9e746eca03ddb/remoting/client/BUILD.gn
[modify] https://crrev.com/715326a623d42be037eca4c333d9e746eca03ddb/remoting/test/BUILD.gn

Project Member

Comment 9 by bugdroid1@chromium.org, May 4 2016

Project Member

Comment 11 by bugdroid1@chromium.org, May 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a86e1e5cef109674814b73cfd606acf6152f2cc

commit 1a86e1e5cef109674814b73cfd606acf6152f2cc
Author: brucedawson <brucedawson@chromium.org>
Date: Thu May 05 05:17:42 2016

Add default manifest for gn nacl64.exe

BUG= 604060 

Review-Url: https://codereview.chromium.org/1945343002
Cr-Commit-Position: refs/heads/master@{#391763}

[modify] https://crrev.com/1a86e1e5cef109674814b73cfd606acf6152f2cc/components/nacl/broker/BUILD.gn

Project Member

Comment 12 by bugdroid1@chromium.org, May 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a51aab488fc2426706646f5d749e54ce8f979fcd

commit a51aab488fc2426706646f5d749e54ce8f979fcd
Author: nacl-deps-roller <nacl-deps-roller@chromium.org>
Date: Thu May 05 06:27:47 2016

Roll src/native_client/ 756d2979a..7965517f3 (1 commit).

https://chromium.googlesource.com/native_client/src/native_client.git/+log/756d2979ac26..7965517f3877

$ git log 756d2979a..7965517f3 --date=short --no-merges --format='%ad %ae %s'
2016-05-04 brucedawson Add default Windows manifest to four .gn executables

BUG= 604060 

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_dbg_32_ng,linux_nacl_sdk_build
TBR=mseaborn@chromium.org

Review-Url: https://codereview.chromium.org/1947043003
Cr-Commit-Position: refs/heads/master@{#391771}

[modify] https://crrev.com/a51aab488fc2426706646f5d749e54ce8f979fcd/DEPS

Project Member

Comment 13 by bugdroid1@chromium.org, May 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf9b781660acfac6f0a13be66d868252d77588d4

commit cf9b781660acfac6f0a13be66d868252d77588d4
Author: brucedawson <brucedawson@chromium.org>
Date: Thu May 05 15:26:37 2016

Add three missing test files to the .gn builds

BUG= 604060 

Review-Url: https://codereview.chromium.org/1949173002
Cr-Commit-Position: refs/heads/master@{#391806}

[modify] https://crrev.com/cf9b781660acfac6f0a13be66d868252d77588d4/google_apis/BUILD.gn

Project Member

Comment 15 by bugdroid1@chromium.org, May 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/443e67e3f6f3037c3c97ccc1dc3fa2e501bfdb15

commit 443e67e3f6f3037c3c97ccc1dc3fa2e501bfdb15
Author: brucedawson <brucedawson@chromium.org>
Date: Mon May 09 20:02:00 2016

Add two missing base tests to gn build

BUG= 604060 

Review-Url: https://codereview.chromium.org/1961493002
Cr-Commit-Position: refs/heads/master@{#392405}

[modify] https://crrev.com/443e67e3f6f3037c3c97ccc1dc3fa2e501bfdb15/base/BUILD.gn

Project Member

Comment 16 by bugdroid1@chromium.org, May 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/63411b4d29eaf35d1dc9c4e9ea48635bd9f34bcd

commit 63411b4d29eaf35d1dc9c4e9ea48635bd9f34bcd
Author: thakis <thakis@chromium.org>
Date: Fri May 13 16:23:29 2016

mac/gn: Add missing test file to base_unittests.

BUG= 604060 

Review-Url: https://codereview.chromium.org/1970343003
Cr-Commit-Position: refs/heads/master@{#393541}

[modify] https://crrev.com/63411b4d29eaf35d1dc9c4e9ea48635bd9f34bcd/base/BUILD.gn

Similar to https://bugs.chromium.org/p/chromium/issues/detail?id=431177#c74 and 75:

thakis@thakis:~/src/chrome/src$ comm -2 -3 <(cd out/Release && ls *tests) <(cd out/gn && ls *tests)
shell_client_lib_unittests

This is looking good on linux!

$ (for t in $(cat tests.txt); do echo $t; diff <(out/gn/$t --gtest_list_tests | grep -v GetParam | sort) <(out/Release/$t --gtest_list_tests | grep -v GetParam | sort); done) > ~/output.txt 

This looks like a lot of tests are missing though.
output.txt
44.0 KB View Download
Project Member

Comment 18 by bugdroid1@chromium.org, May 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94b8620d1d6ed812599213a4e20932b469d66a42

commit 94b8620d1d6ed812599213a4e20932b469d66a42
Author: thakis <thakis@chromium.org>
Date: Tue May 17 02:03:19 2016

gn/linux: Add missing test to events_unittests.

BUG= 604060 

Review-Url: https://codereview.chromium.org/1987553002
Cr-Commit-Position: refs/heads/master@{#394019}

[modify] https://crrev.com/94b8620d1d6ed812599213a4e20932b469d66a42/ui/events/BUILD.gn

Project Member

Comment 19 by bugdroid1@chromium.org, May 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/angle/angle/+/2a9358cc025343df85f97f55f03d8cdbdc64545b

commit 2a9358cc025343df85f97f55f03d8cdbdc64545b
Author: Nico Weber <thakis@chromium.org>
Date: Tue May 17 00:16:11 2016

gn: Add a missing test.

BUG= chromium:604060 

Change-Id: I7e9b8c1bd7cb14daa15575cfcfe9ba3e8a1e6d1b
Reviewed-on: https://chromium-review.googlesource.com/344701
Tryjob-Request: Nico Weber <thakis@chromium.org>
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Commit-Queue: Jamie Madill <jmadill@chromium.org>

[modify] https://crrev.com/2a9358cc025343df85f97f55f03d8cdbdc64545b/src/tests/BUILD.gn

Project Member

Comment 20 by bugdroid1@chromium.org, May 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef5de4c4c1074c888f423b1bd31ec9bb714ea8f0

commit ef5de4c4c1074c888f423b1bd31ec9bb714ea8f0
Author: thakis <thakis@chromium.org>
Date: Tue May 17 16:51:08 2016

gn/linux: Add missing tests to browser_tests.

BUG= 604060 

Review-Url: https://codereview.chromium.org/1985903003
Cr-Commit-Position: refs/heads/master@{#394146}

[modify] https://crrev.com/ef5de4c4c1074c888f423b1bd31ec9bb714ea8f0/chrome/test/BUILD.gn

Project Member

Comment 21 by bugdroid1@chromium.org, May 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/68097aeefdaa344b037824ddc72affe011ff23ad

commit 68097aeefdaa344b037824ddc72affe011ff23ad
Author: geofflang <geofflang@chromium.org>
Date: Tue May 17 18:57:55 2016

Roll ANGLE de44d3a..be815a4

https://chromium.googlesource.com/angle/angle.git/+log/de44d3a..be815a4

BUG= 610800 , chromium:604060 

TBR=jmadill@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.linux:linux_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/1990443002
Cr-Commit-Position: refs/heads/master@{#394180}

[modify] https://crrev.com/68097aeefdaa344b037824ddc72affe011ff23ad/DEPS

Project Member

Comment 22 by bugdroid1@chromium.org, May 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/60dd666a273eb84d361f0fd5de11d770881b46bc

commit 60dd666a273eb84d361f0fd5de11d770881b46bc
Author: fmenozzi <fmenozzi@google.com>
Date: Wed May 18 00:30:06 2016

gn/mac: Add missing test to media_blink_unittests

BUG= 604060 

Review-Url: https://codereview.chromium.org/1983313002
Cr-Commit-Position: refs/heads/master@{#394284}

[modify] https://crrev.com/60dd666a273eb84d361f0fd5de11d770881b46bc/media/blink/BUILD.gn

Project Member

Comment 23 by bugdroid1@chromium.org, May 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0889f3a0a4cee816619636ef78df09f22801068e

commit 0889f3a0a4cee816619636ef78df09f22801068e
Author: thakis <thakis@chromium.org>
Date: Wed May 18 03:39:46 2016

mac/gn: Add missing test to media_unittests.

https://codereview.chromium.org/1748333003 forgot to add this test to gn.
This is the only test missing in media_unittests.

BUG= 584119 , 604060 

Review-Url: https://codereview.chromium.org/1987923003
Cr-Commit-Position: refs/heads/master@{#394325}

[modify] https://crrev.com/0889f3a0a4cee816619636ef78df09f22801068e/media/BUILD.gn

Blockedon: 522654
Project Member

Comment 25 by bugdroid1@chromium.org, May 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69c290c4783727c88f6e1a3a090f4b75554626ff

commit 69c290c4783727c88f6e1a3a090f4b75554626ff
Author: thakis <thakis@chromium.org>
Date: Wed May 18 14:30:44 2016

gn: Add some missing tests to components_unittests.

(It looks like there are more tests missing in components_unittests, but I
don't understand why the other missing tests are missing -- from looking at
the .gn files they should be there, but they aren't.)

BUG= 604060 

Review-Url: https://codereview.chromium.org/1987263002
Cr-Commit-Position: refs/heads/master@{#394414}

[modify] https://crrev.com/69c290c4783727c88f6e1a3a090f4b75554626ff/components/BUILD.gn

Project Member

Comment 26 by bugdroid1@chromium.org, May 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a456f5bda5a89dbe92a59b96a43c3e8dbf59a77d

commit a456f5bda5a89dbe92a59b96a43c3e8dbf59a77d
Author: brucedawson <brucedawson@chromium.org>
Date: Thu May 19 18:32:54 2016

Adding missing wgl_api_unittest.cc to Windows gn build

BUG= 604060 

Review-Url: https://codereview.chromium.org/1996083002
Cr-Commit-Position: refs/heads/master@{#394818}

[modify] https://crrev.com/a456f5bda5a89dbe92a59b96a43c3e8dbf59a77d/ui/gl/BUILD.gn

Remaining missing test files on Windows that I am aware of:

Pending fixes:
bwe_test_logging.cc - it appears that this was unconditionally left in .gyp files by accident, fixes for gyp and gn are in crrev.com/1990373002

child_trace_message_filter_unittest.cc - fix is waiting for approval in crrev.com/1988353003

Probably not needing fixes:
placeholder_unittest.cc - just an empty test needed for gyp, not needed for gn

translate_ui_delegate_unittest.cc - Has conditional in .gn file but not in .gyp file. Was always so. Why? Waiting for a response from https://codereview.chromium.org/1632953002#msg55

That's it.
Project Member

Comment 28 by bugdroid1@chromium.org, May 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04e9985e9f96be6776d3afd3b383e8df3f7fe83e

commit 04e9985e9f96be6776d3afd3b383e8df3f7fe83e
Author: brucedawson <brucedawson@chromium.org>
Date: Fri May 20 17:17:00 2016

Add missing test file to tracing:unit_tests

BUG= 604060 

Review-Url: https://codereview.chromium.org/1988353003
Cr-Commit-Position: refs/heads/master@{#395105}

[modify] https://crrev.com/04e9985e9f96be6776d3afd3b383e8df3f7fe83e/components/tracing/BUILD.gn

Regarding translate_ui_delegate_unittest.cc, crrev.com/1987073003 added that to gn builds:

Description
include translate_ui_delegate_unittest to gn build

BUG=

Committed: https://crrev.com/ccca3fc2b211e33e1390675661fc0c20509be13e
Cr-Commit-Position: refs/heads/master@{#394629}



So, just waiting for review of crrev.com/199037300 and then all tests are aligned.
`components_unittests --gtest_list_tests` has fairly different output in gyp and gn, see comment 17.
I looked at the differences for components_unittests on Windows and found some tests that were gn only but only one set of tests (on Windows anyway) that are gyp only:

ConstrainedWindowViewsTest.
  GrowModalDialogSize
  MaximumBrowserDialogSize
  MaximumWebContentsDialogSize
  ShrinkModalDialogSize

These tests are in constrained_window_views_unittest.cc and the reason I didn't detect this by comparing compile targets is that this source file is built in gn builds, it just isn't built into the tests.

There are two places where "//components/constrained_window" is added to the deps of a build target. I was surprised to find that this causes not just the constrained_window/constrained_window target to build, but also constrained_window/unit_tests. By design?

The reason the tests aren't being compiled in is due to this commented out block in components\BUILD.gn:
    # TODO bug 522654 Enable this when the undefined symbol is fixed in
    # web_modal such that this links.
    #deps += [ "//components/constrained_window:unit_tests" ]

I'll follow up with rouslan@.

There may be similar situations in other test executables - I'll investigate.

Project Member

Comment 32 by bugdroid1@chromium.org, May 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18d45a5ee99333a05c2100a507dcd1c4be978edd

commit 18d45a5ee99333a05c2100a507dcd1c4be978edd
Author: thakis <thakis@chromium.org>
Date: Tue May 31 19:33:57 2016

gn/linux: Add last missing tests to components_unittests.

This seems to just work now.

BUG= 604060 ,522654
TBR=rouslan

Review-Url: https://codereview.chromium.org/2022233002
Cr-Commit-Position: refs/heads/master@{#396896}

[modify] https://crrev.com/18d45a5ee99333a05c2100a507dcd1c4be978edd/components/BUILD.gn

Status: Fixed (was: Assigned)
At the moment, everything's that in a gyp test binary is also in a gn test binary (on linux).
(using the test from comment 17)

Sign in to add a comment