Add support for Zones to assist with unit testing.

[bsutton]

I'm the author of dshell

https://pub.dev/packages/dshell.

I'm using the path package extensively.

I'm currently trying to write unit tests for dshell and having some problems.

Dshell creates a ~/.dshell directory and all my unit tests make adjustments to this directory and its children.

When running unit tests this can be problematic for two reasons.

1. unit tests can't be serialized so different unit tests are writing over each others changes to ~/.dshell and its children
2. Modifying my local file system is of concern as a number of my unit tests do recursive deletes.

As such I'm trying to use Zones and the MemoryFileSystem.
This seems like it will work however the path package causes problems.

The path package has a number of global variables. The key pain point is the 'current' variable that maintains a cache of of the current working directory.

As this variable is global it is shared across zones which means that my unit tests still interfere with each other (e.g. if one test changes the CWD then all unit tests get the new CWD).

It seems to me that we could modify the paths package to be zone aware without much effort.

The 'current' method could be modified to check if it is in the root zone via;
Zone.current == Zone.root

If it is in the root zone it continues as it currently does.
If its not in the root zone then we could modified to pull its settings from a Zone key.

_current = Zone[#paths]._current;

The #paths key would be a unique class that contains all of the global settings that paths uses.

If I submitted a change to make the path package zone aware would it be accepted (assuming suitable quality standards are met)?

I believe these changes could be made without modifying the current api.

[natebosch]

I'm nervous about the idea of having a fake current. A processes working directory is meaningful outside of user code. It would be weird to me if p.current shows one thing, but pwdx <dart VM PID> shows a different thing.

We do already have a hack in this package where current might not match the VM's actual working directory in the case of deleted directories, but I'm not in favor of opening that hack up further.

I'd recommend not changing the working directory of the VM within tests.

What are the specific behaviors you need for where current has a different apparent directory from the processes actual working directory? Can you write your code such that you don't depend on the process working directory at all?

[bsutton]

For existing use cases the 'current' variable work as it does now and always reflect the processes current working directory. If the a path function is called outside of the root zone then we could make it so path looks for a Zone Key e.g. #path. If that key doesn't exist then once again the paths 'current' would still reflect the processes CWD. In this way users can't 'accidently' use an alternate CWD just because they spooled up a Zone. The path code would now only behave differently if they explicitly set the #path key on the zone. I think that should address your concerns about an unexpected variance in the path CWD and the processes CWD. Whilst I agree with you 100% that users should not change the working directory of a process, my use case is a little different. DShell is intended as a replacement for bash. Bash users, rightly or wrongly, are used to changing the CWD. As such Dshell implements global functions like 'cd | push | pop'. The DShell documentation actually recommends against using these functions and recommends using path. This is however not the real issue as DShell works fine with the existing path implementation. The issue for me is explicitly with the unit tests and the need to use a MemoryFileSystem to avoid local file system corruption. I also think using the MemoryFileSystem should be the standard practice when writing unit tests that write to the file system. I'm looking to release a package called something like Test Filesystem. This would use a Zone, the MemoryFileSystem and the path package to make unit testing simpler and avoid local file system pollution. My point here is that I believe this problem and possible solution goes beyond just my requirements. You ask 'Can you write your code such that you don't depend on the process working directory at all?'. In fact most of my unit test do not change the CWD, with the exception of the CD/PUSH/POP tests. However path still causes problems. For example: My cwd is /home/me I start a test zone with the MemoryFileSystem. The MemoryFileSystem CWD is now / So immediately I have two CWD. Hence I need to be able to specify a separate set of settings for path in my new Zone otherwise the path 'current' doesn't match the filesystems CWD. So I would change your original statement: 'It would be weird to me if p.current shows one thing, but pwdx <dart VM PID> shows a different thing.' to 'It would be weird to me if p.current shows one thing, but the *current File system* shows a different thing. That is. Dart allows us to load multiple file systems per process as such it make sense for path to support those use cases and hence needs a CWD per filesystem.

…

On Wed, 11 Dec 2019 at 10:39, Nate Bosch ***@***.***> wrote: I'm nervous about the idea of having a fake current. A processes working directory is meaningful outside of user code. It would be weird to me if p.current shows one thing, but pwdx <dart VM PID> shows a different thing. We do already have a hack in this package where current *might* not match the VM's actual working directory in the case of deleted directories, but I'm not in favor of opening that hack up further. I'd recommend *not* changing the working directory of the VM within tests. What are the specific behaviors you need for where current has a different apparent directory from the processes actual working directory? Can you write your code such that you don't depend on the process working directory at all? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#519?email_source=notifications&email_token=AAG32OGAJP3W7BVMA62XQLDQYASETA5CNFSM4JZFZWY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGRK6TQ#issuecomment-564309838>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OGN24KAKF7HUNOO6RLQYASETANCNFSM4JZFZWYQ> .

[natebosch]

'It would be weird to me if p.current shows one thing, but the current
File system shows a different thing.

We should narrow in on why that happens. Rather than overriding for both the filesystem and this package, the override for the filesystem should work automatically.

When you override the filesystem in a zone, what does Uri.base give you? If it's something other than the correct cwd for that zone we should move this issue to the SDK.

[bsutton]

Am I correct in my reading of the Context code that it doesn't t track the cwd?

The line in the Context code is:

62: String get current => _current ?? p.current;

My reading of this is that if _current is null then read the cwd.
If _current is not null then don't read the cwd.

As such once _current is set for a context it will never be updated.

Is this the intent of the code as it doesn't appear to make any sense.

Also, I think I found a way to track zones without the need to pass a filesystem to the Context.
It looks like the change will be transparent.
I'm about to go on holidays for a week so might be a little while to I get back to you, unless I can convince the wife to let me take the laptop :)

[bsutton]

FYI it looks like Uri.base works correctly in zone with an alternate file system so we only have to manage the fact that path only manages a single cwd.

[bsutton]

Notes for me:

I'm building a stack of zones (optimised for the single zone case) which should allow for the tracking of multiple zones.
I am concerned about futures/microtasks as they can result in flipping between zones in a non-stack like manner. This is likely to mean the stack keeps being rebuilt in an odd order. This could also result in leakage as the stack wont' always be collapsed in a sane manner. Its still might work as mostly it will keep being collapsed. I don't want to use a map as they has the potential to leak memory as we have no way of knowing when a zone goes out of scope.
A better idea might be a constrained map. Allow say a max of 10 zones. These would essentially work as a constrained cache (which is exactly what it is after all). If we use a LRU algorithm we should get reasonable performance and no leakage.

[bsutton]

So I think contexts are meant to be 'fixed' locations that when directly used they cause all path calls to operate relative to that fixed location regardless if the cwd changes for the process.
So I'm thinking I should leave the contexts alone and rather just implement a zone cache directly under the 'current' in path.dart.
This would appear to support the zone requirements whilst having the smallest possible change set.
So implement an LRU map of zone->cwd.
Change path._current from a string to the above lru map.
Have the path.current method lookup the lru for the cwd of the current zone.
If the cwd changes for a zone just write it back to the cache.

[natebosch]

So I think contexts are meant to be 'fixed' locations that when directly used they cause all path calls to operate relative to that fixed location regardless if the cwd changes for the process.

I'm not quite sure about whether this is the intention, it could be.

FYI it looks like Uri.base works correctly in zone with an alternate file system so we only have to manage the fact that path only manages a single cwd.

Assuming the above is the intention I think that means there is nothing we need to do here? The top level p.current will always attempt to read Uri.base which implies it should be correct based on the zone overrides already?

[natebosch]

Another option suggested to me:

Since Context already caches a cwd, you could set up a testing utility outside of this package.

import 'dart:async';

import 'package:path/path.dart' as p;

p.Context get pathContext => Zone.current[#dshellPath] ?? p.context;

T runWithCwd<T>(T Function() body, String current) =>
    runZoned(body, zoneValues: {#dshellPath: p.Context(current: currrent)});

[bsutton]

So I've just done some more testing and Uri.base is failing when used with a zone override in place.

If you call

Directory.current 

with a Zone override in place then the correct Zone cwd is returned.

If you call

Uri.base

then then the code ignores the override.

The problem appears to stem from the fact that Uri.base calls:

Uri _uriBaseClosure() from directory_patch.dart

_uriBaseClosure then calls

_Directory._current

Unlike Directory.current the _Directory version ignores overrides.

So Uri.base as it stands won't work with a Zone override.

So the question now is should Uri.base support Zone overrides or should path not use Uri.base (assuming there is some alternate).

The summary is that the latest patch I've submitted doesn't work.
We could try to detect that we are using a conventional file system and then use Directory.current but I'm uncertain how to do this?

I will have a play with your suggested work around, but it still feels like that path has a problem.
I guess the issues is how overrides are viewed. Are these first class citizens in dart or just a hack for some edge cases?

BTW it would be nice to see some doco around Contexts. If they are part of paths public interface then they should be documented because as they stand I still don't know when/if they should be used.

Brett

[natebosch]

So the question now is should Uri.base support Zone overrides or should path not use Uri.base (assuming there is some alternate).

In my opinion Uri.base should respect Zone overrides.

Do you have a minimal reproducible example showing that Uri.base differs from Dirctory.current? I think with that we can file an SDK issue. Assuming everyone agrees that Uri.base should agree with Directory.current then fixing that there would solve this in the least hacky way.

[bsutton]

I will prep a test.

…

On Sat, 14 Dec 2019, 10:18 am Nate Bosch, ***@***.***> wrote: So the question now is should Uri.base support Zone overrides or should path not use Uri.base (assuming there is some alternate). In my opinion Uri.base should respect Zone overrides. Do you have a minimal reproducible example showing that Uri.base differs from Dirctory.current? I think with that we can file and SDK issue. Assuming everyone agrees that Uri.base *should* agree with Directory.current then fixing that there would solve this in the least hacky way. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#519?email_source=notifications&email_token=AAG32OD3IBFPCJUCSTLTZA3QYQJ5XA5CNFSM4JZFZWY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG3RHII#issuecomment-565646241>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OD6HMYT4VTDMZJB6O3QYQJ5XANCNFSM4JZFZWYQ> .

[bsutton]

Another option suggested to me:

Since Context already caches a cwd, you could set up a testing utility outside of this package.

import 'dart:async';

import 'package:path/path.dart' as p;

p.Context get pathContext => Zone.current[#dshellPath] ?? p.context;

T runWithCwd<T>(T Function() body, String current) =>
    runZoned(body, zoneValues: {#dshellPath: p.Context(current: currrent)});

I've had a better look at this and, if I've understood correctly, I don't think it helps.

If I was writing unit tests that used path directly then sure I can directly call context.xxxx, in the unit test code.

My problem is that I'm writing unit tests for code that uses path.
As such I can't change the code being tested to use an explicit context.

Have I understood this correctly?

[bsutton]

I've submitted an issue here:
dart-lang/sdk#39796