modify MemoryResourceProvider to support windows pathContext for testing #24000

modify MemoryResourceProvider to support windows pathContext for testing #24000

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/c1992fa5d71c298fd2d4fcb16454123c2cadd836

[danrubel, 2015-08-30 02:27:19]

danrubel@google.com changed reviewers:
+ brianwilkerson@google.com

[Brian Wilkerson, 2015-08-30 15:59:17]

brianwilkerson@google.com changed reviewers:
+ paulberry@google.com

[Brian Wilkerson, 2015-08-30 15:59:18]

I seem to remember that there was a reason why we didn't make it platform
dependent when it was first written, but don't remember what that reason was.
I'm hoping that Paul does.

[Paul Berry, 2015-08-30 17:17:39]

On 2015/08/30 15:59:18, Brian Wilkerson wrote:
> I seem to remember that there was a reason why we didn't make it platform
> dependent when it was first written, but don't remember what that reason was.
> I'm hoping that Paul does.

The rationale, IIRC, was that by making the memory file system always use posix
style paths, we could save a lot of time writing and debugging tests, since any
tests that used the memory file system could simply use posix-style paths and
everything would work.  The downside is that any code paths that are tested
using the memory file system don't exercise Windows path support, meaning that
our test coverage for Windows systems is low.  Personally I think we should
change things to use the Windows path context when testing under Windows; we
will spend more time writing and debugging tests, but that will be because we'll
be testing the Windows path logic, so the expense will be worth it. 
Unfortunately, making that change will break a lot of tests, so there will be a
big up front cost when we want to make the switch.  (But there might be clever
ways of mitigating that cost: for example, maybe we could find a way to make the
path context configurable on a per-test basis, and then we could transition the
tests over gradually)

So why didn't this CL break any tests?  Because it doesn't do what the
description says it does.  All it does is replace "posix" with "pathContext",
and MemoryResourceProvider.pathContext is a getter that always returns "posix". 
So the CL has no net effect.

Because of the up front cost of switching MemoryResource provider to use a
Windows path context on Windows, I think we need to step back and find a way of
fixing #24000 without having to do that.  Then we should probably aim for
switching the MemoryResource provider sometime before the release of 1.13.

[danrubel, 2015-08-30 19:00:01]

On 2015/08/30 17:17:39, Paul Berry wrote:
> On 2015/08/30 15:59:18, Brian Wilkerson wrote:
> > I seem to remember that there was a reason why we didn't make it platform
> > dependent when it was first written, but don't remember what that reason
was.
> > I'm hoping that Paul does.
> 
> The rationale, IIRC, was that by making the memory file system always use
posix
> style paths, we could save a lot of time writing and debugging tests, since
any
> tests that used the memory file system could simply use posix-style paths and
> everything would work.  The downside is that any code paths that are tested
> using the memory file system don't exercise Windows path support, meaning that
> our test coverage for Windows systems is low.  Personally I think we should
> change things to use the Windows path context when testing under Windows; we
> will spend more time writing and debugging tests, but that will be because
we'll
> be testing the Windows path logic, so the expense will be worth it. 
> Unfortunately, making that change will break a lot of tests, so there will be
a
> big up front cost when we want to make the switch.  (But there might be clever
> ways of mitigating that cost: for example, maybe we could find a way to make
the
> path context configurable on a per-test basis, and then we could transition
the
> tests over gradually)
> 
> So why didn't this CL break any tests?  Because it doesn't do what the
> description says it does.  All it does is replace "posix" with "pathContext",
> and MemoryResourceProvider.pathContext is a getter that always returns
"posix". 
> So the CL has no net effect.
> 
> Because of the up front cost of switching MemoryResource provider to use a
> Windows path context on Windows, I think we need to step back and find a way
of
> fixing #24000 without having to do that.  Then we should probably aim for
> switching the MemoryResource provider sometime before the release of 1.13.

Correct. The description should have read "Modify MemoryResourceProvider without
modifying existing behavior but such that it can be subclassed and pathContext
overriden for testing Windows paths." ... or something like that. As you
correctly noticed, I did not and do not intend to change any existing behavior.
I just want to subclass it and use that subclass to test Windows specific
behavior in the code completion.

[Paul Berry, 2015-08-30 19:23:32]

On 2015/08/30 19:00:01, danrubel wrote:
> On 2015/08/30 17:17:39, Paul Berry wrote:
> > On 2015/08/30 15:59:18, Brian Wilkerson wrote:
> > > I seem to remember that there was a reason why we didn't make it platform
> > > dependent when it was first written, but don't remember what that reason
> was.
> > > I'm hoping that Paul does.
> > 
> > The rationale, IIRC, was that by making the memory file system always use
> posix
> > style paths, we could save a lot of time writing and debugging tests, since
> any
> > tests that used the memory file system could simply use posix-style paths
and
> > everything would work.  The downside is that any code paths that are tested
> > using the memory file system don't exercise Windows path support, meaning
that
> > our test coverage for Windows systems is low.  Personally I think we should
> > change things to use the Windows path context when testing under Windows; we
> > will spend more time writing and debugging tests, but that will be because
> we'll
> > be testing the Windows path logic, so the expense will be worth it. 
> > Unfortunately, making that change will break a lot of tests, so there will
be
> a
> > big up front cost when we want to make the switch.  (But there might be
clever
> > ways of mitigating that cost: for example, maybe we could find a way to make
> the
> > path context configurable on a per-test basis, and then we could transition
> the
> > tests over gradually)
> > 
> > So why didn't this CL break any tests?  Because it doesn't do what the
> > description says it does.  All it does is replace "posix" with
"pathContext",
> > and MemoryResourceProvider.pathContext is a getter that always returns
> "posix". 
> > So the CL has no net effect.
> > 
> > Because of the up front cost of switching MemoryResource provider to use a
> > Windows path context on Windows, I think we need to step back and find a way
> of
> > fixing #24000 without having to do that.  Then we should probably aim for
> > switching the MemoryResource provider sometime before the release of 1.13.
> 
> Correct. The description should have read "Modify MemoryResourceProvider
without
> modifying existing behavior but such that it can be subclassed and pathContext
> overriden for testing Windows paths." ... or something like that. As you
> correctly noticed, I did not and do not intend to change any existing
behavior.
> I just want to subclass it and use that subclass to test Windows specific
> behavior in the code completion.

Subclassing, of course!  Sorry for not figuring that out.  Carry on, then :)

lgtm

[danrubel, 2015-08-31 18:53:23]

Committed patchset #2 (id:20001) manually as
c1992fa5d71c298fd2d4fcb16454123c2cadd836 (presubmit successful).