grit: Ignore empty file paths when generating input file list

grit: Ignore empty file paths when generating input file list

It is possible for the input path of <structure>
nodes to be empty, so ignore such descendant
nodes when generating the list of input files
at the <grit> root node.

BUG=503635
TEST=GritNodeUnittest.testGetInputFilesChromeScaledImage

[tdanderson, 2015-06-26 00:14:21]

tdanderson@chromium.org changed reviewers:
+ flackr@chromium.org

[tdanderson, 2015-06-26 00:14:22]

Hi Rob, can you please take a look? It seems I forgot this in the previous grit
CL, and it's a prerequisite change for
https://codereview.chromium.org/1197813002/

[flackr, 2015-06-26 00:18:32]

Can you add a test that GetInputFiles generates the correct list given a
ChromeScaledImage with missing assets?

[tdanderson, 2015-06-26 19:30:36]

Rob, can you please take another look?

[flackr, 2015-06-26 19:45:21]

lgtm with test nit.

https://codereview.chromium.org/1212703002/diff/20001/grit/node/misc_unittest.py
File grit/node/misc_unittest.py (right):

https://codereview.chromium.org/1212703002/diff/20001/grit/node/misc_unittest...
grit/node/misc_unittest.py:79: actual.append(split[-2] + '/' + split[-1])
Is this to convert a full path to the last 2 components or relative path? If so
you can use os.relpath relative to where the test files are.

Also, this can be done using a for in python array construction, e.g.
actual = [os.relpath(path, testdir) for path in grd.GetInputFiles()]

[tdanderson, 2015-06-26 20:33:21]

Rob, thanks for the suggestion, that is much nicer. Can you PTAL?

[tdanderson, 2015-06-26 20:35:00]

On 2015/06/26 20:33:21, tdanderson wrote:
> Rob, thanks for the suggestion, that is much nicer. Can you PTAL?

Also, this passes local testing, so if it LGTY then feel free to commit.

[flackr, 2015-06-26 21:18:54]

On 2015/06/26 20:35:00, tdanderson wrote:
> On 2015/06/26 20:33:21, tdanderson wrote:
> > Rob, thanks for the suggestion, that is much nicer. Can you PTAL?
> 
> Also, this passes local testing, so if it LGTY then feel free to commit.

One question / concern, have you tested on Windows? I'm not sure but I'm worried
the path separator may be \ instead of /.

[tdanderson, 2015-06-26 22:34:36]

Good catch, thanks! I tested on Windows and that would have indeed caused a
problem. Patch Set 4 should take care of things.

[flackr, 2015-06-26 23:10:32]

https://codereview.chromium.org/1212703002/diff/60001/grit/node/misc_unittest.py
File grit/node/misc_unittest.py (right):

https://codereview.chromium.org/1212703002/diff/60001/grit/node/misc_unittest...
grit/node/misc_unittest.py:75: actual = [os.path.relpath(path, 'testdata') for
path in grd.GetInputFiles()]
This seems to be dependent on where you're running the test from, I tried
running from the root dir: ./grit.py unit and it failed. I think the path should
be util.PathFromRoot('grit/testdata') as above.

https://codereview.chromium.org/1212703002/diff/60001/grit/node/misc_unittest...
grit/node/misc_unittest.py:76: actual = [path.replace('\\', '/') for path in
actual]
nit: add comment that this is to convert windows paths.

[tdanderson, 2015-06-29 15:34:13]

Rob, can you PTAL?

https://codereview.chromium.org/1212703002/diff/60001/grit/node/misc_unittest.py
File grit/node/misc_unittest.py (right):

https://codereview.chromium.org/1212703002/diff/60001/grit/node/misc_unittest...
grit/node/misc_unittest.py:75: actual = [os.path.relpath(path, 'testdata') for
path in grd.GetInputFiles()]
On 2015/06/26 23:10:32, flackr wrote:
> This seems to be dependent on where you're running the test from, I tried
> running from the root dir: ./grit.py unit and it failed. I think the path
should
> be util.PathFromRoot('grit/testdata') as above.

Done.

https://codereview.chromium.org/1212703002/diff/60001/grit/node/misc_unittest...
grit/node/misc_unittest.py:76: actual = [path.replace('\\', '/') for path in
actual]
On 2015/06/26 23:10:32, flackr wrote:
> nit: add comment that this is to convert windows paths.

Done.

[flackr, 2015-06-29 15:42:27]

LGTM, committed revision 191.

[newt (away), 2015-08-07 23:40:25]

newt@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2015-08-07 23:40:25]

This change broke testGetInputFilesChromeScaledImage() in
grit/node/misc_unittest.py. Could you fix that? Thanks!

[Nico, 2015-08-09 21:48:41]

On 2015/08/07 23:40:25, newt wrote:
> This change broke testGetInputFilesChromeScaledImage() in
> grit/node/misc_unittest.py. Could you fix that? Thanks!

It's broken for me too. Please take a look soon, or revert.

======================================================================
ERROR: testGetInputFilesChromeScaledImage
(grit.node.misc_unittest.GritNodeUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/thakis/src/grit-i18n/grit/node/misc_unittest.py", line 75, in
testGetInputFilesChromeScaledImage
    actual = [os.path.relpath(path, util.PathFromRoot('grit/testdata')) for path
in grd.GetInputFiles()]
  File "/Users/thakis/src/grit-i18n/grit/node/misc.py", line 334, in
GetInputFiles
    input_path = node.GetInputPath()
  File "/Users/thakis/src/grit-i18n/grit/node/structure.py", line 213, in
GetInputPath
    return self.gatherer.GetInputPath()
  File "/Users/thakis/src/grit-i18n/grit/gather/chrome_scaled_image.py", line
137, in GetInputPath
    path, scale, req_scale = self._FindInputFile()
  File "/Users/thakis/src/grit-i18n/grit/gather/chrome_scaled_image.py", line
134, in _FindInputFile
    'Tried ' + self.grd_node.ToRealPath(os.path.join(dir, self.rc_file)))
FileNotFound: The resource file was not found.
  : Tried /Users/thakis/src/grit-i18n/grit/testdata/default_100_percent/a.png

----------------------------------------------------------------------

[tdanderson, 2015-08-10 14:38:30]

On 2015/08/09 21:48:41, Nico (hiding) wrote:
> On 2015/08/07 23:40:25, newt wrote:
> > This change broke testGetInputFilesChromeScaledImage() in
> > grit/node/misc_unittest.py. Could you fix that? Thanks!
> 
> It's broken for me too. Please take a look soon, or revert.
> 
> ======================================================================
> ERROR: testGetInputFilesChromeScaledImage
> (grit.node.misc_unittest.GritNodeUnittest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/Users/thakis/src/grit-i18n/grit/node/misc_unittest.py", line 75, in
> testGetInputFilesChromeScaledImage
>     actual = [os.path.relpath(path, util.PathFromRoot('grit/testdata')) for
path
> in grd.GetInputFiles()]
>   File "/Users/thakis/src/grit-i18n/grit/node/misc.py", line 334, in
> GetInputFiles
>     input_path = node.GetInputPath()
>   File "/Users/thakis/src/grit-i18n/grit/node/structure.py", line 213, in
> GetInputPath
>     return self.gatherer.GetInputPath()
>   File "/Users/thakis/src/grit-i18n/grit/gather/chrome_scaled_image.py", line
> 137, in GetInputPath
>     path, scale, req_scale = self._FindInputFile()
>   File "/Users/thakis/src/grit-i18n/grit/gather/chrome_scaled_image.py", line
> 134, in _FindInputFile
>     'Tried ' + self.grd_node.ToRealPath(os.path.join(dir, self.rc_file)))
> FileNotFound: The resource file was not found.
>   : Tried /Users/thakis/src/grit-i18n/grit/testdata/default_100_percent/a.png
> 
> ----------------------------------------------------------------------

Looking now.

[tdanderson, 2015-08-10 14:59:39]

On 2015/08/10 14:38:30, tdanderson wrote:
> On 2015/08/09 21:48:41, Nico (hiding) wrote:
> > On 2015/08/07 23:40:25, newt wrote:
> > > This change broke testGetInputFilesChromeScaledImage() in
> > > grit/node/misc_unittest.py. Could you fix that? Thanks!
> > 
> > It's broken for me too. Please take a look soon, or revert.
> > 
> > ======================================================================
> > ERROR: testGetInputFilesChromeScaledImage
> > (grit.node.misc_unittest.GritNodeUnittest)
> > ----------------------------------------------------------------------
> > Traceback (most recent call last):
> >   File "/Users/thakis/src/grit-i18n/grit/node/misc_unittest.py", line 75, in
> > testGetInputFilesChromeScaledImage
> >     actual = [os.path.relpath(path, util.PathFromRoot('grit/testdata')) for
> path
> > in grd.GetInputFiles()]
> >   File "/Users/thakis/src/grit-i18n/grit/node/misc.py", line 334, in
> > GetInputFiles
> >     input_path = node.GetInputPath()
> >   File "/Users/thakis/src/grit-i18n/grit/node/structure.py", line 213, in
> > GetInputPath
> >     return self.gatherer.GetInputPath()
> >   File "/Users/thakis/src/grit-i18n/grit/gather/chrome_scaled_image.py",
line
> > 137, in GetInputPath
> >     path, scale, req_scale = self._FindInputFile()
> >   File "/Users/thakis/src/grit-i18n/grit/gather/chrome_scaled_image.py",
line
> > 134, in _FindInputFile
> >     'Tried ' + self.grd_node.ToRealPath(os.path.join(dir, self.rc_file)))
> > FileNotFound: The resource file was not found.
> >   : Tried
/Users/thakis/src/grit-i18n/grit/testdata/default_100_percent/a.png
> > 
> > ----------------------------------------------------------------------
> 
> Looking now.

The issue here is that the three png files in this CL
do not actually exist in the repository, hence the
FileNotFound. Re-adding these locally fixes the test.

Rob, is there any chance these could have
been skipped when you committed revision 191? I assume
if they were committed and then subsequently deleted
it would have shown up in git log.

Note also that the PNGs in this CL aren't actually
valid PNGs; they're just nonempty text files that I
gave a .png extension to. Could they have been
rejected by a submit script for that reason?

[flackr, 2015-08-10 15:30:47]

On 2015/08/10 14:59:39, tdanderson wrote:
> On 2015/08/10 14:38:30, tdanderson wrote:
> > On 2015/08/09 21:48:41, Nico (hiding) wrote:
> > > On 2015/08/07 23:40:25, newt wrote:
> > > > This change broke testGetInputFilesChromeScaledImage() in
> > > > grit/node/misc_unittest.py. Could you fix that? Thanks!
> > > 
> > > It's broken for me too. Please take a look soon, or revert.
> > > 
> > > ======================================================================
> > > ERROR: testGetInputFilesChromeScaledImage
> > > (grit.node.misc_unittest.GritNodeUnittest)
> > > ----------------------------------------------------------------------
> > > Traceback (most recent call last):
> > >   File "/Users/thakis/src/grit-i18n/grit/node/misc_unittest.py", line 75,
in
> > > testGetInputFilesChromeScaledImage
> > >     actual = [os.path.relpath(path, util.PathFromRoot('grit/testdata'))
for
> > path
> > > in grd.GetInputFiles()]
> > >   File "/Users/thakis/src/grit-i18n/grit/node/misc.py", line 334, in
> > > GetInputFiles
> > >     input_path = node.GetInputPath()
> > >   File "/Users/thakis/src/grit-i18n/grit/node/structure.py", line 213, in
> > > GetInputPath
> > >     return self.gatherer.GetInputPath()
> > >   File "/Users/thakis/src/grit-i18n/grit/gather/chrome_scaled_image.py",
> line
> > > 137, in GetInputPath
> > >     path, scale, req_scale = self._FindInputFile()
> > >   File "/Users/thakis/src/grit-i18n/grit/gather/chrome_scaled_image.py",
> line
> > > 134, in _FindInputFile
> > >     'Tried ' + self.grd_node.ToRealPath(os.path.join(dir, self.rc_file)))
> > > FileNotFound: The resource file was not found.
> > >   : Tried
> /Users/thakis/src/grit-i18n/grit/testdata/default_100_percent/a.png
> > > 
> > > ----------------------------------------------------------------------
> > 
> > Looking now.
> 
> The issue here is that the three png files in this CL
> do not actually exist in the repository, hence the
> FileNotFound. Re-adding these locally fixes the test.
> 
> Rob, is there any chance these could have
> been skipped when you committed revision 191? I assume
> if they were committed and then subsequently deleted
> it would have shown up in git log.
> 
> Note also that the PNGs in this CL aren't actually
> valid PNGs; they're just nonempty text files that I
> gave a .png extension to. Could they have been
> rejected by a submit script for that reason?

My bad, I've added the missing files: revision 194.

[newt (away), 2015-08-10 16:35:59]

Thanks for fixing!

[tdanderson, 2015-08-10 17:17:19]

On 2015/08/10 16:35:59, newt wrote:
> Thanks for fixing!

Thanks for fixing this, Rob.
grit roll: https://codereview.chromium.org/1285603003/