Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2789433004:
Add tools for code coverage support in iOS. (Closed)
|
Created:
3 years, 8 months ago by lpromero Modified:
3 years, 7 months ago Reviewers:
baxley, brucedawson, Eugene But (OOO till 7-30), marq (ping after 24h), lindsayw, Nico, sdefresne, Paweł Hajdan Jr., liaoyuke CC:
chromium-reviews, ios-reviews_chromium.org, vmpstr+watch_chromium.org, mac-reviews_chromium.org, marq+watch_chromium.org, lpromero+watch_chromium.org, ios-reviews+showcase_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd tools for code coverage support in iOS.
In this CL, support is added to all unit tests targets and the Showcase
Earl Grey tests.
BUG=676034
R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org
Review-Url: https://codereview.chromium.org/2789433004
Cr-Commit-Position: refs/heads/master@{#469301}
Committed: https://chromium.googlesource.com/chromium/src/+/df93a8860b4379bb27d05bb0368189cf8e0f3a2b
Patch Set 1 #
Total comments: 18
Patch Set 2 : Feedback #
Total comments: 5
Patch Set 3 : Refinement #Patch Set 4 : Enable code coverage for the entire sources. #
Total comments: 2
Patch Set 5 : Feedback #
Total comments: 4
Patch Set 6 : Feedback #
Total comments: 4
Patch Set 7 : Nits #
Total comments: 2
Patch Set 8 : Rebased #
Messages
Total messages: 62 (31 generated)
lpromero@chromium.org changed reviewers: + baxley@chromium.org
This is WIP, but at least it works. https://codereview.chromium.org/2789433004/diff/1/base/test/test_support_ios.mm File base/test/test_support_ios.mm (right): https://codereview.chromium.org/2789433004/diff/1/base/test/test_support_ios.... base/test/test_support_ios.mm:173: //coverage_util::SetupIfNecessary(); I will re-enable this once I add the "-fprofile-instr-generate" ldflag to all the targets that depend on test_support_ios.mm. If it's not in this CL, I will clean this up. https://codereview.chromium.org/2789433004/diff/1/build/secondary/testing/gte... File build/secondary/testing/gtest/BUILD.gn (right): https://codereview.chromium.org/2789433004/diff/1/build/secondary/testing/gte... build/secondary/testing/gtest/BUILD.gn:101: if (is_ios && gtest_include_ios_coverage) { "gtest_include_ios_coverage" is mentioned here, for context in a comment below. https://codereview.chromium.org/2789433004/diff/1/ios/showcase/BUILD.gn File ios/showcase/BUILD.gn (right): https://codereview.chromium.org/2789433004/diff/1/ios/showcase/BUILD.gn#newco... ios/showcase/BUILD.gn:22: ldflags = [ "-fprofile-instr-generate" ] I am not sure this is needed on the non test targets like here the Showcase app. https://codereview.chromium.org/2789433004/diff/1/ios/showcase/BUILD.gn#newco... ios/showcase/BUILD.gn:64: ldflags = [ "-fprofile-instr-generate" ] Should I instead add to ios_eg_test template and use a variable to selectively opt-into code coverage? https://codereview.chromium.org/2789433004/diff/1/ios/showcase/core/BUILD.gn File ios/showcase/core/BUILD.gn (right): https://codereview.chromium.org/2789433004/diff/1/ios/showcase/core/BUILD.gn#... ios/showcase/core/BUILD.gn:24: "-fcoverage-mapping", Can this be added with a config, like enable_arc below? https://codereview.chromium.org/2789433004/diff/1/ios/showcase/test/showcase_... File ios/showcase/test/showcase_test_case.mm (right): https://codereview.chromium.org/2789433004/diff/1/ios/showcase/test/showcase_... ios/showcase/test/showcase_test_case.mm:19: coverage_util::SetupIfNecessary(); We will also need to add this call to the non-Showcase related targets. Do EG tests use test_support_ios.mm? If so it would be enough. https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.h File testing/coverage_util_ios.h (right): https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.h... testing/coverage_util_ios.h:10: // In debug builds, if ENABLE_TEST_CODE_COVERAGE is defined, sets the filename Sylvain, you told me GN was against such env variable. Can you advise on a solution to replace? There is already "gtest_include_ios_coverage", will it be used in EG tests? https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.mm File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.m... testing/coverage_util_ios.mm:12: #if !defined(NDEBUG)// && defined(ENABLE_TEST_CODE_COVERAGE) I commented the ENABLE_TEST_CODE_COVERAGE variable. Should I replace it with something more GN friendly?
lpromero@chromium.org changed reviewers: + eugenebut@chromium.org
+eugenebut
lgtm! https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.h File testing/coverage_util_ios.h (right): https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.h... testing/coverage_util_ios.h:12: void SetupIfNecessary(); Should this be SetUpForDebugBuild() https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.mm File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.m... testing/coverage_util_ios.mm:13: static dispatch_once_t onceToken; s/onceToken/once_token Same Style comment for the rest of variables https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.m... testing/coverage_util_ios.mm:25: __llvm_profile_set_filename([fileName UTF8String]); Use sys_string_conversions ? https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.m... testing/coverage_util_ios.mm:28: NSLog(@"Coverage data at %@.", fileName); Should this be DLOG instead?
https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.h File testing/coverage_util_ios.h (right): https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.h... testing/coverage_util_ios.h:12: void SetupIfNecessary(); On 2017/03/31 18:27:51, Eugene But wrote: > Should this be SetUpForDebugBuild() Well, there would probably be another condition: ENABLE_TEST_CODE_COVERAGE or its successor in gn world, hence the "if necessary". Do you have another idea for including both conditions? https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.mm File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.m... testing/coverage_util_ios.mm:13: static dispatch_once_t onceToken; On 2017/03/31 18:27:51, Eugene But wrote: > s/onceToken/once_token > > Same Style comment for the rest of variables Since this is now a .mm file, isn't it Objective-C style that applies? Or because we are in a free function, we use C++ style? I change the style anyway, just curious. https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.m... testing/coverage_util_ios.mm:25: __llvm_profile_set_filename([fileName UTF8String]); On 2017/03/31 18:27:51, Eugene But wrote: > Use sys_string_conversions ? So there is a catch22 :) base depends on //testing, so //testing can't depend on //base. There is even a presubmit check that made the same comment as you! https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.m... testing/coverage_util_ios.mm:28: NSLog(@"Coverage data at %@.", fileName); On 2017/03/31 18:27:51, Eugene But wrote: > Should this be DLOG instead? Idem, I can't use base/logging.h.
https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.h File testing/coverage_util_ios.h (right): https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.h... testing/coverage_util_ios.h:12: void SetupIfNecessary(); On 2017/03/31 20:35:20, lpromero wrote: > On 2017/03/31 18:27:51, Eugene But wrote: > > Should this be SetUpForDebugBuild() > > Well, there would probably be another condition: ENABLE_TEST_CODE_COVERAGE or > its successor in gn world, hence the "if necessary". Do you have another idea > for including both conditions? Nope, I guess IfNecessary is fine. Sorry my previous comment supposed to be a question. With a question mark :) https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.mm File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/1/testing/coverage_util_ios.m... testing/coverage_util_ios.mm:13: static dispatch_once_t onceToken; On 2017/03/31 20:35:20, lpromero wrote: > On 2017/03/31 18:27:51, Eugene But wrote: > > s/onceToken/once_token > > > > Same Style comment for the rest of variables > > Since this is now a .mm file, isn't it Objective-C style that applies? Or > because we are in a free function, we use C++ style? I change the style anyway, > just curious. C++ Style is used inside C-functions in C++ namespaces: https://cs.chromium.org/chromium/src/styleguide/objective-c/objective-c.md?q=...
Thanks also for the screen shots (.png's) those are awesome :) https://codereview.chromium.org/2789433004/diff/20001/docs/ios/coverage.md File docs/ios/coverage.md (right): https://codereview.chromium.org/2789433004/diff/20001/docs/ios/coverage.md#ne... docs/ios/coverage.md:24: 1. To see the **line coverage** for *all the instrumented source files*: Sorry if this is obvious, but why do all of these points start with #1? https://codereview.chromium.org/2789433004/diff/20001/ios/showcase/core/BUILD.gn File ios/showcase/core/BUILD.gn (right): https://codereview.chromium.org/2789433004/diff/20001/ios/showcase/core/BUILD... ios/showcase/core/BUILD.gn:25: ] Do "-g" or "-Wall" need to be added anywhere?
Should we update the CL description to include a summary of the work that is done to make the tool work? i.e. it's not simply adding showcase code coverage. LGTM https://codereview.chromium.org/2789433004/diff/20001/testing/coverage_util_i... File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/20001/testing/coverage_util_i... testing/coverage_util_ios.mm:12: #if !defined(NDEBUG) // && defined(ENABLE_TEST_CODE_COVERAGE) nit: do we want the commented out condition?
Description was changed from ========== Add code coverage to Showcase. BUG=676034 R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org ========== to ========== Add tools for code coverage support in iOS. As an example, support is added to the Showcase app. BUG=676034 R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org ==========
Updated the description and the approach. Still some changes needed. https://codereview.chromium.org/2789433004/diff/20001/docs/ios/coverage.md File docs/ios/coverage.md (right): https://codereview.chromium.org/2789433004/diff/20001/docs/ios/coverage.md#ne... docs/ios/coverage.md:24: 1. To see the **line coverage** for *all the instrumented source files*: On 2017/04/03 15:32:09, lindsayw wrote: > Sorry if this is obvious, but why do all of these points start with #1? Markdown rendering changes it to the required number when displaying. It's useful in lists, as that way you can insert new list items without having to shift the numbers. There is an example at https://guides.github.com/features/mastering-markdown/. https://codereview.chromium.org/2789433004/diff/20001/ios/showcase/core/BUILD.gn File ios/showcase/core/BUILD.gn (right): https://codereview.chromium.org/2789433004/diff/20001/ios/showcase/core/BUILD... ios/showcase/core/BUILD.gn:25: ] On 2017/04/03 15:32:09, lindsayw wrote: > Do "-g" or "-Wall" need to be added anywhere? Initially I had them but seems it worked without. I think it's because they are already set by default when building for out/Debug-iphonesimulator.
lgtm Thanks for the clarifications!
RS LGTM given other LGTMs.
The CQ bit was checked by lpromero@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.
sdefresne, this is ready for review
Can you update your screenshot? https://codereview.chromium.org/2789433004/diff/60001/docs/ios/coverage.md File docs/ios/coverage.md (right): https://codereview.chromium.org/2789433004/diff/60001/docs/ios/coverage.md#ne... docs/ios/coverage.md:3: 1. Setup: Code coverage is only supported for Debug-iphonesimulator builds. I think you can add a "Coverage" config in src/ios/build/tools/setup-gn.py (as discussed offline). Once this is done, developer won't have anything to setup, but instead can just select the correct configuration in Xcode (pro: building and testing for coverage would not destroy what was previously built). Note that it would still be good to document the "ios_enable_coverage" variable here.
lpromero@chromium.org changed reviewers: + liaoyuke@chromium.org
The CQ bit was checked by lpromero@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...
PTAL https://codereview.chromium.org/2789433004/diff/60001/docs/ios/coverage.md File docs/ios/coverage.md (right): https://codereview.chromium.org/2789433004/diff/60001/docs/ios/coverage.md#ne... docs/ios/coverage.md:3: 1. Setup: Code coverage is only supported for Debug-iphonesimulator builds. On 2017/04/27 08:26:26, sdefresne wrote: > I think you can add a "Coverage" config in src/ios/build/tools/setup-gn.py (as > discussed offline). Once this is done, developer won't have anything to setup, > but instead can just select the correct configuration in Xcode (pro: building > and testing for coverage would not destroy what was previously built). Done. > Note that it would still be good to document the "ios_enable_coverage" variable > here. What kind of info about ios_enable_coverage would you see fit here?
https://codereview.chromium.org/2789433004/diff/80001/testing/coverage_util_i... File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/80001/testing/coverage_util_i... testing/coverage_util_ios.mm:15: void SetupIfNecessary() { Maybe name this "ConfigureOutputPathIfCoverageEnabled".
Addressed feedback. https://codereview.chromium.org/2789433004/diff/80001/testing/coverage_util_i... File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/80001/testing/coverage_util_i... testing/coverage_util_ios.mm:15: void SetupIfNecessary() { On 2017/04/27 15:34:28, lpromero wrote: > Maybe name this "ConfigureOutputPathIfCoverageEnabled". Seems like this describes more the implementation than the goal. Let say tomorrow I want to add another condition, I will have to rename the method.
lpromero@chromium.org changed reviewers: + phajdan.jr@chromium.org
phajdan.jr@chromium.org: Please review changes in base and testing.
Description was changed from ========== Add tools for code coverage support in iOS. As an example, support is added to the Showcase app. BUG=676034 R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org ========== to ========== Add tools for code coverage support in iOS. In this CL, support is added to all unit tests targets and the Showcase Earl Grey tests. BUG=676034 R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2789433004/diff/80001/testing/coverage_util_i... File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/80001/testing/coverage_util_i... testing/coverage_util_ios.mm:15: void SetupIfNecessary() { On 2017/04/27 15:44:26, lpromero wrote: > On 2017/04/27 15:34:28, lpromero wrote: > > Maybe name this "ConfigureOutputPathIfCoverageEnabled". > > Seems like this describes more the implementation than the goal. Let say > tomorrow I want to add another condition, I will have to rename the method. I agree. I think "ConfigureCoverageReportPath" is good. It does not depends on the implementation (whether coverage is enabled, ...) while making it clear what this function does (configure the path where the coverage report will be saved).
The CQ bit was checked by lpromero@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...
https://codereview.chromium.org/2789433004/diff/80001/testing/coverage_util_i... File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/80001/testing/coverage_util_i... testing/coverage_util_ios.mm:15: void SetupIfNecessary() { On 2017/04/28 08:06:25, sdefresne wrote: > On 2017/04/27 15:44:26, lpromero wrote: > > On 2017/04/27 15:34:28, lpromero wrote: > > > Maybe name this "ConfigureOutputPathIfCoverageEnabled". > > > > Seems like this describes more the implementation than the goal. Let say > > tomorrow I want to add another condition, I will have to rename the method. > > I agree. I think "ConfigureCoverageReportPath" is good. It does not depends on > the implementation (whether coverage is enabled, ...) while making it clear what > this function does (configure the path where the coverage report will be saved). Done. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/nits https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... testing/coverage_util_ios.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... testing/coverage_util_ios.mm:35: #endif nit: Add // !defined(NDEBUG) && BUILDFLAG(IOS_ENABLE_COVERAGE)
On 2017/04/28 11:15:37, Paweł Hajdan Jr. wrote: > LGTM w/nits > > https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... > File testing/coverage_util_ios.mm (right): > > https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... > testing/coverage_util_ios.mm:1: // Copyright 2014 The Chromium Authors. All > rights reserved. > nit: 2017 > > https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... > testing/coverage_util_ios.mm:35: #endif > nit: Add // !defined(NDEBUG) && BUILDFLAG(IOS_ENABLE_COVERAGE) lgtm! Could you please wrap CL descriptions to 72 chars?
Description was changed from ========== Add tools for code coverage support in iOS. In this CL, support is added to all unit tests targets and the Showcase Earl Grey tests. BUG=676034 R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org ========== to ========== Add tools for code coverage support in iOS. In this CL, support is added to all unit tests targets and the Showcase Earl Grey tests. BUG=676034 R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org ==========
The CQ bit was checked by lpromero@chromium.org to run a CQ dry run
https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... File testing/coverage_util_ios.mm (right): https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... testing/coverage_util_ios.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/04/28 11:15:37, Paweł Hajdan Jr. wrote: > nit: 2017 Done. https://codereview.chromium.org/2789433004/diff/100001/testing/coverage_util_... testing/coverage_util_ios.mm:35: #endif On 2017/04/28 11:15:37, Paweł Hajdan Jr. wrote: > nit: Add // !defined(NDEBUG) && BUILDFLAG(IOS_ENABLE_COVERAGE) Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lpromero@chromium.org changed reviewers: + brucedawson@chromium.org
brucedawson@chromium.org: Please review changes in build/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lpromero@chromium.org changed reviewers: + thakis@chromium.org
+Nico for build/
build/ generally lgtm, but: https://codereview.chromium.org/2789433004/diff/120001/build/secondary/testin... File build/secondary/testing/gtest/BUILD.gn (right): https://codereview.chromium.org/2789433004/diff/120001/build/secondary/testin... build/secondary/testing/gtest/BUILD.gn:109: "../coverage_util_ios.mm", We're actively trying to modify this file, see recent changes in https://crbug.com/630705 -- could you maybe wait with touching this one until this file is figured out? (Or, if this merges cleanly with the 1.8.0 change, then you don't have to wait. I don't remember exactly how this file here is being modified in that bug, please check.)
Thanks for the heads-up! https://codereview.chromium.org/2789433004/diff/120001/build/secondary/testin... File build/secondary/testing/gtest/BUILD.gn (right): https://codereview.chromium.org/2789433004/diff/120001/build/secondary/testin... build/secondary/testing/gtest/BUILD.gn:109: "../coverage_util_ios.mm", On 2017/05/03 15:12:10, Nico wrote: > We're actively trying to modify this file, see recent changes in > https://crbug.com/630705 -- could you maybe wait with touching this one until > this file is figured out? (Or, if this merges cleanly with the 1.8.0 change, > then you don't have to wait. I don't remember exactly how this file here is > being modified in that bug, please check.) There is only a disclaimer at the top: https://codereview.chromium.org/2852613002/diff/40001/build/secondary/testing... But it might have a conflict with the surrounding lines and need a simple merge. I see the other CL was reverted twice already, is there an estimation for when I can go through with my CL?
On Wed, May 3, 2017 at 11:20 AM, <lpromero@chromium.org> wrote: > Thanks for the heads-up! > > > https://codereview.chromium.org/2789433004/diff/120001/ > build/secondary/testing/gtest/BUILD.gn > File build/secondary/testing/gtest/BUILD.gn (right): > > https://codereview.chromium.org/2789433004/diff/120001/ > build/secondary/testing/gtest/BUILD.gn#newcode109 > build/secondary/testing/gtest/BUILD.gn:109: "../coverage_util_ios.mm", > On 2017/05/03 15:12:10, Nico wrote: > > We're actively trying to modify this file, see recent changes in > > https://crbug.com/630705 -- could you maybe wait with touching this > one until > > this file is figured out? (Or, if this merges cleanly with the 1.8.0 > change, > > then you don't have to wait. I don't remember exactly how this file > here is > > being modified in that bug, please check.) > > There is only a disclaimer at the top: > https://codereview.chromium.org/2852613002/diff/40001/ > build/secondary/testing/gtest/BUILD.gn > But it might have a conflict with the surrounding lines and need a > simple merge. > I see the other CL was reverted twice already, is there an estimation > for when I can go through with my CL? > Once it sticks :-) Or if it hasn't stuck by end of this week, or you're in a rush for some reason, I suppose landing earlier is fine too. But hopefully the roll will reland today and then stay in. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
pwnall says he's fine with rebasing, so you can land this. On May 3, 2017 11:26 AM, "Nico Weber" <thakis@chromium.org> wrote: > On Wed, May 3, 2017 at 11:20 AM, <lpromero@chromium.org> wrote: > >> Thanks for the heads-up! >> >> >> https://codereview.chromium.org/2789433004/diff/120001/build >> /secondary/testing/gtest/BUILD.gn >> File build/secondary/testing/gtest/BUILD.gn (right): >> >> https://codereview.chromium.org/2789433004/diff/120001/build >> /secondary/testing/gtest/BUILD.gn#newcode109 >> build/secondary/testing/gtest/BUILD.gn:109: "../coverage_util_ios.mm", >> On 2017/05/03 15:12:10, Nico wrote: >> > We're actively trying to modify this file, see recent changes in >> > https://crbug.com/630705 -- could you maybe wait with touching this >> one until >> > this file is figured out? (Or, if this merges cleanly with the 1.8.0 >> change, >> > then you don't have to wait. I don't remember exactly how this file >> here is >> > being modified in that bug, please check.) >> >> There is only a disclaimer at the top: >> https://codereview.chromium.org/2852613002/diff/40001/build/ >> secondary/testing/gtest/BUILD.gn >> But it might have a conflict with the surrounding lines and need a >> simple merge. >> I see the other CL was reverted twice already, is there an estimation >> for when I can go through with my CL? >> > > Once it sticks :-) Or if it hasn't stuck by end of this week, or you're in > a rush for some reason, I suppose landing earlier is fine too. But > hopefully the roll will reland today and then stay in. > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by lpromero@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 checked by lpromero@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, baxley@chromium.org, lindsayw@chromium.org, marq@chromium.org, sdefresne@chromium.org, phajdan.jr@chromium.org, liaoyuke@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2789433004/#ps140001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493884409466520,
"parent_rev": "b6bec5fd85d27f68ba6ecdee9217ee0638e391ab", "commit_rev":
"df93a8860b4379bb27d05bb0368189cf8e0f3a2b"}
Message was sent while issue was closed.
Description was changed from ========== Add tools for code coverage support in iOS. In this CL, support is added to all unit tests targets and the Showcase Earl Grey tests. BUG=676034 R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org ========== to ========== Add tools for code coverage support in iOS. In this CL, support is added to all unit tests targets and the Showcase Earl Grey tests. BUG=676034 R=sdefresne@chromium.org,marq@chromium.org,lindsayw@chromium.org Review-Url: https://codereview.chromium.org/2789433004 Cr-Commit-Position: refs/heads/master@{#469301} Committed: https://chromium.googlesource.com/chromium/src/+/df93a8860b4379bb27d05bb03681... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/df93a8860b4379bb27d05bb03681...
Message was sent while issue was closed.
On 2017/05/04 01:18:54, Nico wrote: > pwnall says he's fine with rebasing, so you can land this. Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
