Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1526933002:
Add no-compile test support to GN build (Closed)
DescriptionAdd no-compile test support to GN build
No-compile tests have been disabled for a while on GYP build, and GN
didn't support it.
This CL converts //build/nocompile.gypi to //build/nocompile.gni to
support no-compile test, and splits out test cases from base_unittests
to base_nocompile_tests on GN build.
This doesn't enable the test by default. To opt-in to it, set
enable_nocompile_tests to true in GN args.
BUG=568478
Committed: https://crrev.com/0137dc5d4031d8bc13d28adf579a9af6a2169c75
Cr-Commit-Position: refs/heads/master@{#365757}
Patch Set 1 #
Total comments: 3
Patch Set 2 : +comments #
Messages
Total messages: 23 (10 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1526933002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1526933002/1
Description was changed from ========== Add no-compile test support to GN build No-compile tests have been disabled for a while on GYP build, and GN didn't support it. This CL converts //build/nocompile.gypi to //build/nocompile.gni to support no-compile test, and splits out test cases from base_unittests to base_nocompile_tests on GN build. This doesn't enable the test by default. To opt-in to it, set enable_nocompile_tests to true in GN args. BUG= ========== to ========== Add no-compile test support to GN build No-compile tests have been disabled for a while on GYP build, and GN didn't support it. This CL converts //build/nocompile.gypi to //build/nocompile.gni to support no-compile test, and splits out test cases from base_unittests to base_nocompile_tests on GN build. This doesn't enable the test by default. To opt-in to it, set enable_nocompile_tests to true in GN args. BUG=568478 ==========
tzik@chromium.org changed reviewers: + dcheng@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1526933002/diff/1/build/nocompile.gni File build/nocompile.gni (right): https://codereview.chromium.org/1526933002/diff/1/build/nocompile.gni#newcode75 build/nocompile.gni:75: "4", I'm not very familiar with GN: what does the 4 here mean?
dcheng@chromium.org changed reviewers: + danakj@chromium.org
Also, this LGTM. +danakj as a //base OWNER
dcheng@chromium.org changed reviewers: + thakis@chromium.org
Oh... this has changes in //build to. +thakis instead =)
Do we really need nocompile tests? They're a pain to deal with, and from the bug it sounds like they wouldn't have caught much interesting stuff. (And since they're opt-in, they won't catch regressions anyhow.) If you feel that they do add value, then this lgtm. But I'd prefer if we could get rid of them. https://codereview.chromium.org/1526933002/diff/1/build/nocompile.gni File build/nocompile.gni (right): https://codereview.chromium.org/1526933002/diff/1/build/nocompile.gni#newcode75 build/nocompile.gni:75: "4", On 2015/12/15 18:14:47, dcheng wrote: > I'm not very familiar with GN: what does the 4 here mean? See this on the lhs: '4', # number of compilers to invoke in parallel. It's just passed through literally. (see also `gn help action_forach`). Maybe a comment saying "# Number of parallel compiles" on the rhs would be good too.
In my opinion, the nocompile tests have enough value that I think it's worth keeping them. Though we don't run them often, it's useful to verify the static assert coverage in base::Bind, base::Callback. Otherwise, whoever's changing them ends up manually 'testing' them locally and having to revert all those changes before sending it through the trybots, etc. This ends up being a pretty laborious and error-prone process.
On 2015/12/15 19:10:04, Nico wrote: > Do we really need nocompile tests? They're a pain to deal with, and from the bug > it sounds like they wouldn't have caught much interesting stuff. (And since > they're opt-in, they won't catch regressions anyhow.) > > If you feel that they do add value, then this lgtm. But I'd prefer if we could > get rid of them. > Yes, I think it's important to verify static_assert and other feature that disables features. Missing GN support makes me less motivated to run it regulary :p. However, I also feel most of these tests can/should be moved from no-compile test to the regular base_unittests. We are currently checking the banned type directly in base::Bind, and check compilation failure in a .nc. But we can move the logic to a predicate and check the value both in base::Bind and a regular test without .nc.
https://codereview.chromium.org/1526933002/diff/1/build/nocompile.gni File build/nocompile.gni (right): https://codereview.chromium.org/1526933002/diff/1/build/nocompile.gni#newcode75 build/nocompile.gni:75: "4", On 2015/12/15 19:10:04, Nico wrote: > On 2015/12/15 18:14:47, dcheng wrote: > > I'm not very familiar with GN: what does the 4 here mean? > > See this on the lhs: > > '4', # number of compilers to invoke in parallel. > > It's just passed through literally. (see also `gn help action_forach`). Maybe a > comment saying "# Number of parallel compiles" on the rhs would be good too. Added the comment too. Yes, it's from the .gypi.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1526933002/#ps20001 (title: "+comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1526933002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1526933002/20001
Message was sent while issue was closed.
Description was changed from ========== Add no-compile test support to GN build No-compile tests have been disabled for a while on GYP build, and GN didn't support it. This CL converts //build/nocompile.gypi to //build/nocompile.gni to support no-compile test, and splits out test cases from base_unittests to base_nocompile_tests on GN build. This doesn't enable the test by default. To opt-in to it, set enable_nocompile_tests to true in GN args. BUG=568478 ========== to ========== Add no-compile test support to GN build No-compile tests have been disabled for a while on GYP build, and GN didn't support it. This CL converts //build/nocompile.gypi to //build/nocompile.gni to support no-compile test, and splits out test cases from base_unittests to base_nocompile_tests on GN build. This doesn't enable the test by default. To opt-in to it, set enable_nocompile_tests to true in GN args. BUG=568478 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add no-compile test support to GN build No-compile tests have been disabled for a while on GYP build, and GN didn't support it. This CL converts //build/nocompile.gypi to //build/nocompile.gni to support no-compile test, and splits out test cases from base_unittests to base_nocompile_tests on GN build. This doesn't enable the test by default. To opt-in to it, set enable_nocompile_tests to true in GN args. BUG=568478 ========== to ========== Add no-compile test support to GN build No-compile tests have been disabled for a while on GYP build, and GN didn't support it. This CL converts //build/nocompile.gypi to //build/nocompile.gni to support no-compile test, and splits out test cases from base_unittests to base_nocompile_tests on GN build. This doesn't enable the test by default. To opt-in to it, set enable_nocompile_tests to true in GN args. BUG=568478 Committed: https://crrev.com/0137dc5d4031d8bc13d28adf579a9af6a2169c75 Cr-Commit-Position: refs/heads/master@{#365757} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0137dc5d4031d8bc13d28adf579a9af6a2169c75 Cr-Commit-Position: refs/heads/master@{#365757} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
