Remove _unit_test_config hack, instead use FileSystem() in cpp.py & cpp_unittest.py

Remove _unit_test_config hack, instead use FileSystem() in cpp.py & cpp_unittest.py

Tools/Scripts/webkitpy/style/checkers/cpp*.py scripts use _unit_test_config global variable as a scaffholding, to imitate opening of mock header {contents}. It is a hack and should be avoided. Instead we should use filesystem (FileSystem()) object for the purpose.

BUG=305507
TEST=None; no behavior change;

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160363

[r.kasibhatla, 2013-10-21 14:31:44]

PTAL.

[Dirk Pranke, 2013-10-21 18:53:31]

I very much like the idea of getting rid of this hack, but I think this is
probably the wrong way to do it. We shouldn't have code calling into codecs (or
your "io" object) at all outside of webkitpy/common/system. 

It looks like the calling code in cpp.py can be replaced w/
host.filesystem.read_text_file(), with the possible exception that the error
handling is 'replace' instead of 'strict'. I'm not sure if that's necessary.

I'd be inclined to change the code in cpp.py to use the filesystem object, and
then override read_text_file as necessary to mock things out as needed.

Does that make sense?

[r.kasibhatla, 2013-10-21 19:59:08]

On 2013/10/21 18:53:31, Dirk Pranke wrote:
> I very much like the idea of getting rid of this hack, but I think this is
> probably the wrong way to do it. We shouldn't have code calling into codecs
(or
> your "io" object) at all outside of webkitpy/common/system. 
> 
> It looks like the calling code in cpp.py can be replaced w/
> host.filesystem.read_text_file(), with the possible exception that the error
> handling is 'replace' instead of 'strict'. I'm not sure if that's necessary.
> 
> I'd be inclined to change the code in cpp.py to use the filesystem object, and
> then override read_text_file as necessary to mock things out as needed.
> 
> Does that make sense?
You are suggesting that in cpp.py/cpp_unittest.py we should directly invoke
webkitpy/common/system/filesystem.py:read_text_file(), for reading the mock
header content. If needed, we should override/add new version of
read_text_file() to return mock header content. Is it correct?
Couple of doubts over this - 
1. We would still need to create the MockIo object - for faking the presence of
header. 
2. We need to get handle to MockIo object (created in cpp_unittest.py in cpp.py)
so that filesystem.read_text_file can return the content (without opening the
file).
Is my understanding correct?

[Dirk Pranke, 2013-10-21 22:11:27]

On Mon, Oct 21, 2013 at 12:59 PM, <r.kasibhatla@samsung.com> wrote:

> On 2013/10/21 18:53:31, Dirk Pranke wrote:
>
>> I very much like the idea of getting rid of this hack, but I think this is
>> probably the wrong way to do it. We shouldn't have code calling into
>> codecs
>>
> (or
>
>> your "io" object) at all outside of webkitpy/common/system.
>>
>
>  It looks like the calling code in cpp.py can be replaced w/
>> host.filesystem.read_text_**file(), with the possible exception that the
>> error
>> handling is 'replace' instead of 'strict'. I'm not sure if that's
>> necessary.
>>
>
>  I'd be inclined to change the code in cpp.py to use the filesystem
>> object, and
>> then override read_text_file as necessary to mock things out as needed.
>>
>
>  Does that make sense?
>>
> You are suggesting that in cpp.py/cpp_unittest.py we should directly
> invoke
> webkitpy/common/system/**filesystem.py:read_text_file()**, for reading
> the mock
> header content. If needed, we should override/add new version of
> read_text_file() to return mock header content. Is it correct?
>

Yes, that's the basic idea.


> Couple of doubts over this -
> 1. We would still need to create the MockIo object - for faking the
> presence of
> header.
>

No, you should be able to override the read_text_file() routine on the
host.filesystem object instead (in the test code, save the original method
into a temp value, and create a stub routine that returns one thing for the
header and calls into the original method for anything else).


> 2. We need to get handle to MockIo object (created in cpp_unittest.py in
> cpp.py)
> so that filesystem.read_text_file can return the content (without opening
> the
> file).
>

You could override/use open_text_file_for_reading instead, but I'd just
rewrite the code in cpp.py to split the string returned from
read_text_file() and loop over that. The performance difference should be
insignificant.

You should no longer need MockIo objects or anything exposing a file-like
interface.

At least, that's the basic idea I had. I didn't see anything that would
keep this from working, but it's possible I missed something.

-- Dirk

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[r.kasibhatla, 2013-10-22 14:27:31]

On 2013/10/21 22:11:27, Dirk Pranke wrote:
> On Mon, Oct 21, 2013 at 12:59 PM, <mailto:r.kasibhatla@samsung.com> wrote:
> 
> > On 2013/10/21 18:53:31, Dirk Pranke wrote:
> >
> >> I very much like the idea of getting rid of this hack, but I think this is
> >> probably the wrong way to do it. We shouldn't have code calling into
> >> codecs
> >>
> > (or
> >
> >> your "io" object) at all outside of webkitpy/common/system.
> >>
> >
> >  It looks like the calling code in cpp.py can be replaced w/
> >> host.filesystem.read_text_**file(), with the possible exception that the
> >> error
> >> handling is 'replace' instead of 'strict'. I'm not sure if that's
> >> necessary.
> >>
> >
> >  I'd be inclined to change the code in cpp.py to use the filesystem
> >> object, and
> >> then override read_text_file as necessary to mock things out as needed.
> >>
> >
> >  Does that make sense?
> >>
> > You are suggesting that in cpp.py/cpp_unittest.py we should directly
> > invoke
> > webkitpy/common/system/**filesystem.py:read_text_file()**, for reading
> > the mock
> > header content. If needed, we should override/add new version of
> > read_text_file() to return mock header content. Is it correct?
> >
> 
> Yes, that's the basic idea.
> 
> 
> > Couple of doubts over this -
> > 1. We would still need to create the MockIo object - for faking the
> > presence of
> > header.
> >
> 
> No, you should be able to override the read_text_file() routine on the
> host.filesystem object instead (in the test code, save the original method
> into a temp value, and create a stub routine that returns one thing for the
> header and calls into the original method for anything else).
> 
> 
> > 2. We need to get handle to MockIo object (created in cpp_unittest.py in
> > cpp.py)
> > so that filesystem.read_text_file can return the content (without opening
> > the
> > file).
> >
> 
> You could override/use open_text_file_for_reading instead, but I'd just
> rewrite the code in cpp.py to split the string returned from
> read_text_file() and loop over that. The performance difference should be
> insignificant.
> 
> You should no longer need MockIo objects or anything exposing a file-like
> interface.
> 
> At least, that's the basic idea I had. I didn't see anything that would
> keep this from working, but it's possible I missed something.

@dpranke: Can you check the new implementation?

1. As per discussion, I have replaced all the calls to io.open() where io=codecs
or MockIo with FileSystem.read_text_file().

2. For unittest cases, I am pointing the FileSystem.read_text_file() to a
mock_read_text_file() which will just return the mock_header_contents. In
general, we will invoke FileSystem.read_text_file() for reading any file/header
and getting contents.

3. For verification, test_webkitpy doesn't show any regressions. I ran
check-webkit-style also randomly on various files & I didn't observe any
regressions either.

One implementation aspect which I couldn't change was invoking
FileSystem.read_text_file() in cpp.py, especially with the call of
cpp.py:process_file_data() is coming from cpp_unittest.py. Since, we need the
FileSystem object with modified func ptr for read_text_file, the only way I
could fathom, was storing the FileSystem object into CppChecker (as class level
member) and invoking it directly from required functions. Any other suggestions
for it?

[Dirk Pranke, 2013-10-22 19:17:02]

This is okay (lgtm); it's definitely a step in the right direction.

It shouldn't be too hard to propagate the actual filesystem object down through
the static functions to where you need it, but you would need to update the
unittests as well, of course.

https://codereview.chromium.org/32663002/diff/70001/Tools/Scripts/webkitpy/co...
File Tools/Scripts/webkitpy/common/system/filesystem.py (right):

https://codereview.chromium.org/32663002/diff/70001/Tools/Scripts/webkitpy/co...
Tools/Scripts/webkitpy/common/system/filesystem.py:209: def read_text_file(self,
path, errors='strict'):
Are you sure we need this? I.e., does test-webkitpy include test cases that need
replaced content?

[r.kasibhatla, 2013-10-22 21:56:12]

On 2013/10/22 19:17:02, Dirk Pranke wrote:
> This is okay (lgtm); it's definitely a step in the right direction.
> 
> It shouldn't be too hard to propagate the actual filesystem object down
through
> the static functions to where you need it, but you would need to update the
> unittests as well, of course.

Can you throw your idea?

> 
>
https://codereview.chromium.org/32663002/diff/70001/Tools/Scripts/webkitpy/co...
> File Tools/Scripts/webkitpy/common/system/filesystem.py (right):
> 
>
https://codereview.chromium.org/32663002/diff/70001/Tools/Scripts/webkitpy/co...
> Tools/Scripts/webkitpy/common/system/filesystem.py:209: def
read_text_file(self,
> path, errors='strict'):
> Are you sure we need this? I.e., does test-webkitpy include test cases that
need
> replaced content?

Just ensured that current calls of open with 'replace' were continued. Not
really checked whether we need it or not. Will check and if not required revert
the changes to filesystem.py.

[Dirk Pranke, 2013-10-22 22:16:29]

On 2013/10/22 21:56:12, r.kasibhatla wrote:
> 
> Can you throw your idea?
> 

I'm not sure what you mean by "can you throw your idea" :) basically, it
involves creating a filesystem object roughly where we should create it (e.g.,
in CppChecker.__init__(), although really it should be passed in from
check-webkit-style), pass it through all of the functions it calls until we get
to the functions that need the filesystem, and then update the unittests to pass
in the mocks accordingly.

That said, it's not super-high-priority. Land this change when you're ready and
I may or may not get around to posting the above if I have some free time.

[r.kasibhatla, 2013-10-23 04:44:09]

On 2013/10/22 22:16:29, Dirk Pranke wrote:
> On 2013/10/22 21:56:12, r.kasibhatla wrote:
> > 
> > Can you throw your idea?
> > 
> 
> I'm not sure what you mean by "can you throw your idea" :) basically, it
> involves creating a filesystem object roughly where we should create it (e.g.,
> in CppChecker.__init__(), although really it should be passed in from
> check-webkit-style), pass it through all of the functions it calls until we
get
> to the functions that need the filesystem, and then update the unittests to
pass
> in the mocks accordingly.

I thought of passing all the way to the desired function, but felt it might be
unwarranted considering it will change the signatures of lots of functions and
will be including an argument which most of the functions are not even bothered
to use. If you see, even now we are passing FileSystem object all the way from
unittests to creation of CppChecker object and passing it further means
modifying another set of functions though they don't require it. So, basically I
wanted to check your thoughts on solving the passing of filesystem.

> 
> That said, it's not super-high-priority. Land this change when you're ready
and
> I may or may not get around to posting the above if I have some free time.

I agree and will further check on this later and will put new changes to correct
it.
Also, I have checked and the changes to filesystem.py are not required. We don't
require the 'strict'|'replace' mode option. So, I have reverted the changes to
filesystem.py and uploaded the patch on ToT. Can you stamp it?

[r.kasibhatla, 2013-10-23 07:49:39]

Corrected the summary of the change.
Can somebody stamp the changes.

[Dirk Pranke, 2013-10-23 17:42:51]

still lgtm.

https://codereview.chromium.org/32663002/diff/140001/Tools/Scripts/webkitpy/s...
File Tools/Scripts/webkitpy/style/checkers/cpp.py (right):

https://codereview.chromium.org/32663002/diff/140001/Tools/Scripts/webkitpy/s...
Tools/Scripts/webkitpy/style/checkers/cpp.py:4019: CppChecker.fs = fs or
FileSystem()
Note that this would be problematic if we had multiple CppChecker objects
simultaneously active and yet using different filesystem objects. Fortunately,
(a) we don't normally, and (b) there's no real state in the (non-mock) fs
objects, so it shouldn't be an issue. Still a good reason to make this be
instance fields and not class fields.

[Dirk Pranke, 2013-10-23 17:44:28]

On 2013/10/23 04:44:09, r.kasibhatla wrote:
> I thought of passing all the way to the desired function, but felt it might be
> unwarranted considering it will change the signatures of lots of functions and
> will be including an argument which most of the functions are not even
bothered
> to use. If you see, even now we are passing FileSystem object all the way from
> unittests to creation of CppChecker object and passing it further means
> modifying another set of functions though they don't require it. So, basically
I
> wanted to check your thoughts on solving the passing of filesystem.
> 

Yup, you have to touch a lot of methods just to pass the object through. This is
one of the few reasons to prefer making all these things methods on an object
rather than bare functions. The tradeoff is generally worth it in the long run,
IMO.

[commit-bot: I haz the power, 2013-10-23 18:11:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/32663002/140001

[commit-bot: I haz the power, 2013-10-23 19:31:31]

Change committed as 160363