Add functionality to find xctest files under application target.

Add functionality to find xctest files under application target.

Previously, our generated Xcode project has the issue that Xcode couldn't find
our xctests. And to fix it, one must first find the list of xctest files
under each of the application targets and then associate them with the
corresponding xctest target.

This CL enables identifying application targets and also find the list of xctest
files (xctests.mm and egtest.mm) under them recursively.

BUG=614818
Committed: https://crrev.com/043a15ddcc6c864d6b68ba471c330b0bcdccbf0d
Cr-Commit-Position: refs/heads/master@{#441482}

[liaoyuke, 2016-12-13 07:00:13]

Description was changed from

==========
Add functionality to find earl grey files under XCTest targets.



BUG=
==========

to

==========
Add functionality to find earl grey files under app or XCTest bundle.

To fix the Xcode test navigator integration issue, one needs to first associate
earl grey files with corresponding application or XCTest bundle. This CL enables
identifying application or XCTest bundles and also find the list of earl grey 
files under them recursively.

BUG=614818
==========

[liaoyuke, 2016-12-13 07:00:13]

liaoyuke@chromium.org changed reviewers:
+ justincohen@chromium.org, sdefresne@chromium.org

[liaoyuke, 2016-12-13 07:18:27]

Hi Sylvain, Justin,

Please take a look.

Thank you very much!

[liaoyuke, 2016-12-14 07:41:36]

On 2016/12/13 07:18:27, liaoyuke wrote:
> Hi Sylvain, Justin,
> 
> Please take a look.
> 
> Thank you very much!

ping?

[sdefresne, 2016-12-14 13:00:03]

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:37: typedef std::unordered_map<const Target*,
Target::FileList> TargetToFileList;
nit: it is recommend to use "using" instead of "typedef"

using TargetToFileList = std::unordered_map<const Target*, Target::FileList>;

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:110: if (eg_files_per_target->find(target) !=
eg_files_per_target->end()) {
No braces here (since there are not used in the rest of the file).

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:126: if (!deps_eg_files.empty()) {
No braces here.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:127: AppendAndRemoveDuplicates(&eg_files,
deps_eg_files);
I think it would be better to just add everything and remove duplicates at the
end.

  for (...) {
    ...
    const Target::FileList& deps_eg_files = (*eg_files_per_target)[target.ptr];
    eg_files.insert(eg_files.end(), deps_eg_files.begin(), deps_eg_files.end());
  }

Since you are already removing the duplicates at the end, there is no point
removing them at each iteration of the loop.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:146: // Find the list of earl grey files recursively
under each of the application
nit: Find -> Finds

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:148: void GetEarlGreyFilesForAll(
I'm not sure we need to check source file in xctest_targets since in Chromium we
only put the tests in the application itself, not in the module. Can you check
whether you get the same result if you do not iterate on xctest_targets, and if
this is the case, just remove the xctest_targets from this method?

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:151: std::vector<Target::FileList>* file_lists) {
DCHECK_EQ(xctest_targets.size(), application_targets.size());

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:167: // Returns the corresponding application target
given a XCTest target. Returns
I would change this function to DCHECK if it does not find the application
target for an XCTest.

// Returns the application target given an XCTest target.
const Target* FindApplicationTarget(const Target* xctest_target,
                                    const std::vector<const Target*>& targets) {
  DCHECK(IsXCTestTarget(xctest_target));
  DCHECK(base::EndsWith(xctest_target->label().name(),
kXCTestTargetNamePostfix));
  std::string application_target_name = xctest_target->label().name().substr(
      0, xctest_target->label().name().size() -
strlen(kXCTestTargetNamePostfix));
  for (const Target* target : targets) {
    if (target->label().name() == application_target_name) {
      return target;
    }
  }
  NOTREACHED();
  return nullptr;
}

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:369: if (target->output_type() !=
Target::CREATE_BUNDLE) {
nit: I would use an helper method IsXCTestTarget here:

namespace {
bool IsXCTestTarget(const Target* target) {
  return target->output_type() == Target::CREATE_BUNDLE &&
      target->bundle_data().product_type() ==
"com.apple.product-type.bundle.unit-test";
}
}

  ...
  for (const Target* target : targets) {
    if (!IsXCTestTarget(target))
      continue;

    xctest_targets->push_back(target);
    application_targets->ush_back(FindApplicationTarget(target, targets));
  }

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.h
File tools/gn/xcode_writer.h (right):

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.h...
tools/gn/xcode_writer.h:68: // corresonding application targets.
nit: corresonding -> corresponding

[liaoyuke, 2016-12-14 21:46:50]

Description was changed from

==========
Add functionality to find earl grey files under app or XCTest bundle.

To fix the Xcode test navigator integration issue, one needs to first associate
earl grey files with corresponding application or XCTest bundle. This CL enables
identifying application or XCTest bundles and also find the list of earl grey 
files under them recursively.

BUG=614818
==========

to

==========
Add functionality to find earl grey files under application target.

Previously, our generated project has the issue that Xcode couldn't find our
earl grey tests. And to fix it, one must first find the list of earl grey files
under each of the application targets and then associate them with the
corresponding xctest target.

This CL enables identifying application targets and also find the list of earl
grey files under them recursively.

BUG=614818
==========

[liaoyuke, 2016-12-15 00:02:27]

Patchset #5 (id:80001) has been deleted

[liaoyuke, 2016-12-15 00:03:12]

Addressed comments.

PTAL!

Thank you very much!

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:37: typedef std::unordered_map<const Target*,
Target::FileList> TargetToFileList;
On 2016/12/14 13:00:03, sdefresne wrote:
> nit: it is recommend to use "using" instead of "typedef"
> 
> using TargetToFileList = std::unordered_map<const Target*, Target::FileList>;

Done.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:110: if (eg_files_per_target->find(target) !=
eg_files_per_target->end()) {
On 2016/12/14 13:00:03, sdefresne wrote:
> No braces here (since there are not used in the rest of the file).

Done.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:126: if (!deps_eg_files.empty()) {
On 2016/12/14 13:00:02, sdefresne wrote:
> No braces here.

Done.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:127: AppendAndRemoveDuplicates(&eg_files,
deps_eg_files);
On 2016/12/14 13:00:03, sdefresne wrote:
> I think it would be better to just add everything and remove duplicates at the
> end.
> 
>   for (...) {
>     ...
>     const Target::FileList& deps_eg_files =
(*eg_files_per_target)[target.ptr];
>     eg_files.insert(eg_files.end(), deps_eg_files.begin(),
deps_eg_files.end());
>   }
> 
> Since you are already removing the duplicates at the end, there is no point
> removing them at each iteration of the loop.

Done.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:146: // Find the list of earl grey files recursively
under each of the application
On 2016/12/14 13:00:02, sdefresne wrote:
> nit: Find -> Finds

Done.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:148: void GetEarlGreyFilesForAll(
On 2016/12/14 13:00:02, sdefresne wrote:
> I'm not sure we need to check source file in xctest_targets since in Chromium
we
> only put the tests in the application itself, not in the module. Can you check
> whether you get the same result if you do not iterate on xctest_targets, and
if
> this is the case, just remove the xctest_targets from this method?

You are right, I have double-checked, there is no earl grey files under xctest
targets.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:151: std::vector<Target::FileList>* file_lists) {
On 2016/12/14 13:00:03, sdefresne wrote:
> DCHECK_EQ(xctest_targets.size(), application_targets.size());

Acknowledged.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:167: // Returns the corresponding application target
given a XCTest target. Returns
On 2016/12/14 13:00:03, sdefresne wrote:
> I would change this function to DCHECK if it does not find the application
> target for an XCTest.
> 
> // Returns the application target given an XCTest target.
> const Target* FindApplicationTarget(const Target* xctest_target,
>                                     const std::vector<const Target*>& targets)
{
>   DCHECK(IsXCTestTarget(xctest_target));
>   DCHECK(base::EndsWith(xctest_target->label().name(),
> kXCTestTargetNamePostfix));
>   std::string application_target_name = xctest_target->label().name().substr(
>       0, xctest_target->label().name().size() -
> strlen(kXCTestTargetNamePostfix));
>   for (const Target* target : targets) {
>     if (target->label().name() == application_target_name) {
>       return target;
>     }
>   }
>   NOTREACHED();
>   return nullptr;
> }

Acknowledged.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.c...
tools/gn/xcode_writer.cc:167: // Returns the corresponding application target
given a XCTest target. Returns
On 2016/12/14 13:00:03, sdefresne wrote:
> I would change this function to DCHECK if it does not find the application
> target for an XCTest.
> 
> // Returns the application target given an XCTest target.
> const Target* FindApplicationTarget(const Target* xctest_target,
>                                     const std::vector<const Target*>& targets)
{
>   DCHECK(IsXCTestTarget(xctest_target));
>   DCHECK(base::EndsWith(xctest_target->label().name(),
> kXCTestTargetNamePostfix));
>   std::string application_target_name = xctest_target->label().name().substr(
>       0, xctest_target->label().name().size() -
> strlen(kXCTestTargetNamePostfix));
>   for (const Target* target : targets) {
>     if (target->label().name() == application_target_name) {
>       return target;
>     }
>   }
>   NOTREACHED();
>   return nullptr;
> }

Thank you for the note. I will this logic when I write the function to find
xctest target given application target.

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.h
File tools/gn/xcode_writer.h (right):

https://codereview.chromium.org/2574643002/diff/60001/tools/gn/xcode_writer.h...
tools/gn/xcode_writer.h:68: // corresonding application targets.
On 2016/12/14 13:00:03, sdefresne wrote:
> nit: corresonding -> corresponding

Done.

[justincohen, 2016-12-15 00:30:24]

https://codereview.chromium.org/2574643002/diff/100001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2574643002/diff/100001/tools/gn/xcode_writer....
tools/gn/xcode_writer.cc:37: 
Is there a better, less sticky way of finding Xctests?  Do we need to worry
about xctests that aren't eg?

[liaoyuke, 2016-12-19 08:03:23]

PTAL!

I know you are pretty busy with upstreaming. So, no rush, take your time to
review.

Thank you very much!

[sdefresne, 2016-12-21 12:46:07]

https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer....
tools/gn/xcode_writer.cc:94: void SearchEarlGreyFiles(const Target* target,
Maybe we can make this a bit more general and look for egtest.mm or xctest.mm
files, and rename FindXCTestFileForTargets(...).

https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer....
tools/gn/xcode_writer.cc:357: XcodeWriter::FilterApplicationTargets(targets,
&application_targets);
Previously the code was doing:

1. look for all xctest module targets,
2. search for the app targets bundling those xctest targets,
3. look for all egtests in both set of targets.

I suggested changing step 3. to just look for egtest files in the second list of
targets, but instead you just removed step 1. and look for egtest file in all
targets. This is IMO less correct (as if another target links an egtest file the
corresponding test won't be run).

Can you restore the step 1. and update step 2. to only get the application
targets corresponding to those xctests. Something like:

std::vector<const Target*> xctest_module_targets;
FindXCTestModuleTargets(targets, &xctest_module_targets);

std::vector<const Target*> xctest_application_targets;
FindXCTestApplicationTargets(targets, xctest_module_targets,
&xctest_application_targets);

std::vector<Target::FileList> xctest_file_lists;
FindXCTestFileForTargets(xctest_application_targets, xctest_file_lists);

[liaoyuke, 2016-12-21 18:23:25]

Patchset #8 (id:160001) has been deleted

[liaoyuke, 2016-12-21 18:25:19]

PTAL.

Thank you very much!

https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer.cc
File tools/gn/xcode_writer.cc (right):

https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer....
tools/gn/xcode_writer.cc:94: void SearchEarlGreyFiles(const Target* target,
On 2016/12/21 12:46:07, sdefresne ooo till 2nd Jan 17 wrote:
> Maybe we can make this a bit more general and look for egtest.mm or xctest.mm
> files, and rename FindXCTestFileForTargets(...).

Done.

https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer....
tools/gn/xcode_writer.cc:357: XcodeWriter::FilterApplicationTargets(targets,
&application_targets);
On 2016/12/21 12:46:07, sdefresne ooo till 2nd Jan 17 wrote:
> Previously the code was doing:
> 
> 1. look for all xctest module targets,
> 2. search for the app targets bundling those xctest targets,
> 3. look for all egtests in both set of targets.
> 
> I suggested changing step 3. to just look for egtest files in the second list
of
> targets, but instead you just removed step 1. and look for egtest file in all
> targets. This is IMO less correct (as if another target links an egtest file
the
> corresponding test won't be run).
> 
> Can you restore the step 1. and update step 2. to only get the application
> targets corresponding to those xctests. Something like:
> 
> std::vector<const Target*> xctest_module_targets;
> FindXCTestModuleTargets(targets, &xctest_module_targets);
> 
> std::vector<const Target*> xctest_application_targets;
> FindXCTestApplicationTargets(targets, xctest_module_targets,
> &xctest_application_targets);
> 
> std::vector<Target::FileList> xctest_file_lists;
> FindXCTestFileForTargets(xctest_application_targets, xctest_file_lists);

Done.

[liaoyuke, 2017-01-03 17:35:23]

On 2016/12/21 18:25:19, liaoyuke wrote:
> PTAL.
> 
> Thank you very much!
> 
>
https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer.cc
> File tools/gn/xcode_writer.cc (right):
> 
>
https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer....
> tools/gn/xcode_writer.cc:94: void SearchEarlGreyFiles(const Target* target,
> On 2016/12/21 12:46:07, sdefresne ooo till 2nd Jan 17 wrote:
> > Maybe we can make this a bit more general and look for egtest.mm or
xctest.mm
> > files, and rename FindXCTestFileForTargets(...).
> 
> Done.
> 
>
https://codereview.chromium.org/2574643002/diff/140001/tools/gn/xcode_writer....
> tools/gn/xcode_writer.cc:357: XcodeWriter::FilterApplicationTargets(targets,
> &application_targets);
> On 2016/12/21 12:46:07, sdefresne ooo till 2nd Jan 17 wrote:
> > Previously the code was doing:
> > 
> > 1. look for all xctest module targets,
> > 2. search for the app targets bundling those xctest targets,
> > 3. look for all egtests in both set of targets.
> > 
> > I suggested changing step 3. to just look for egtest files in the second
list
> of
> > targets, but instead you just removed step 1. and look for egtest file in
all
> > targets. This is IMO less correct (as if another target links an egtest file
> the
> > corresponding test won't be run).
> > 
> > Can you restore the step 1. and update step 2. to only get the application
> > targets corresponding to those xctests. Something like:
> > 
> > std::vector<const Target*> xctest_module_targets;
> > FindXCTestModuleTargets(targets, &xctest_module_targets);
> > 
> > std::vector<const Target*> xctest_application_targets;
> > FindXCTestApplicationTargets(targets, xctest_module_targets,
> > &xctest_application_targets);
> > 
> > std::vector<Target::FileList> xctest_file_lists;
> > FindXCTestFileForTargets(xctest_application_targets, xctest_file_lists);
> 
> Done.

PING

[Justin Cohen (wrong one), 2017-01-03 17:38:38]

justincohen@google.com changed reviewers:
+ justincohen@google.com

[Justin Cohen (wrong one), 2017-01-03 17:38:39]

Please update CL description to clarify this captures xctests.mm and egtest.mm

[liaoyuke, 2017-01-03 17:46:43]

Description was changed from

==========
Add functionality to find earl grey files under application target.

Previously, our generated project has the issue that Xcode couldn't find our
earl grey tests. And to fix it, one must first find the list of earl grey files
under each of the application targets and then associate them with the
corresponding xctest target.

This CL enables identifying application targets and also find the list of earl
grey files under them recursively.

BUG=614818
==========

to

==========
Add functionality to find xctest files under application target.

Previously, our generated Xcode project has the issue that Xcode couldn't find 
our xctests. And to fix it, one must first find the list of xctest files
under each of the application targets and then associate them with the
corresponding xctest target.

This CL enables identifying application targets and also find the list of xctest
files (xctests.mm and egtest.mm) under them recursively.

BUG=614818
==========

[liaoyuke, 2017-01-03 17:47:37]

Done. Thank you for catching this!

On Tue, Jan 3, 2017 at 9:38 AM <justincohen@google.com> wrote:

> Please update CL description to clarify this captures xctests.mm and
> egtest.mm
>
> https://codereview.chromium.org/2574643002/
>

-- 
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.

[sdefresne, 2017-01-04 14:13:05]

lgtm

[liaoyuke, 2017-01-04 17:20:12]

liaoyuke@chromium.org changed reviewers:
+ dpranke@chromium.org

[liaoyuke, 2017-01-04 17:20:27]

On 2017/01/04 14:13:05, sdefresne wrote:
> lgtm

Thank you for reviewing :)

Hello Dirk,

Could you please take a look for owner's approval?

Thank you very much!

[Dirk Pranke, 2017-01-04 20:24:43]

lgtm

[liaoyuke, 2017-01-04 20:25:45]

Thank you :)

On Wed, Jan 4, 2017 at 12:24 PM <dpranke@chromium.org> wrote:

> lgtm
>
> https://codereview.chromium.org/2574643002/
>

-- 
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.

[liaoyuke, 2017-01-04 21:55:11]

The CQ bit was checked by liaoyuke@chromium.org

[commit-bot: I haz the power, 2017-01-04 21:55:35]

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-04 22:10:33]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1483566911750910,
"parent_rev": "c8c01e824d0276ebd32695088bb4a4a084dc0bb1", "commit_rev":
"a6c74df25d2451316c2f3866ab606707769a9c2b"}

[commit-bot: I haz the power, 2017-01-04 22:11:09]

Description was changed from

==========
Add functionality to find xctest files under application target.

Previously, our generated Xcode project has the issue that Xcode couldn't find 
our xctests. And to fix it, one must first find the list of xctest files
under each of the application targets and then associate them with the
corresponding xctest target.

This CL enables identifying application targets and also find the list of xctest
files (xctests.mm and egtest.mm) under them recursively.

BUG=614818
==========

to

==========
Add functionality to find xctest files under application target.

Previously, our generated Xcode project has the issue that Xcode couldn't find 
our xctests. And to fix it, one must first find the list of xctest files
under each of the application targets and then associate them with the
corresponding xctest target.

This CL enables identifying application targets and also find the list of xctest
files (xctests.mm and egtest.mm) under them recursively.

BUG=614818

Review-Url: https://codereview.chromium.org/2574643002
==========

[commit-bot: I haz the power, 2017-01-04 22:11:10]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2017-01-04 22:14:33]

Description was changed from

==========
Add functionality to find xctest files under application target.

Previously, our generated Xcode project has the issue that Xcode couldn't find 
our xctests. And to fix it, one must first find the list of xctest files
under each of the application targets and then associate them with the
corresponding xctest target.

This CL enables identifying application targets and also find the list of xctest
files (xctests.mm and egtest.mm) under them recursively.

BUG=614818

Review-Url: https://codereview.chromium.org/2574643002
==========

to

==========
Add functionality to find xctest files under application target.

Previously, our generated Xcode project has the issue that Xcode couldn't find
our xctests. And to fix it, one must first find the list of xctest files
under each of the application targets and then associate them with the
corresponding xctest target.

This CL enables identifying application targets and also find the list of xctest
files (xctests.mm and egtest.mm) under them recursively.

BUG=614818

Committed: https://crrev.com/043a15ddcc6c864d6b68ba471c330b0bcdccbf0d
Cr-Commit-Position: refs/heads/master@{#441482}
==========

[commit-bot: I haz the power, 2017-01-04 22:14:34]

Patchset 8 (id:??) landed as
https://crrev.com/043a15ddcc6c864d6b68ba471c330b0bcdccbf0d
Cr-Commit-Position: refs/heads/master@{#441482}