-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Zones to assist with unit testing. #55
Comments
I'm nervous about the idea of having a fake We do already have a hack in this package where I'd recommend not changing the working directory of the VM within tests. What are the specific behaviors you need for where |
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
<#55?email_source=notifications&email_token=AAG32OGAJP3W7BVMA62XQLDQYASETA5CNFSM4JZFZWY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGRK6TQ#issuecomment-564309838>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OGN24KAKF7HUNOO6RLQYASETANCNFSM4JZFZWYQ>
.
|
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 |
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. 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. |
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. |
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. |
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.
Assuming the above is the intention I think that means there is nothing we need to do here? The top level |
Another option suggested to me: Since 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)}); |
So I've just done some more testing and Uri.base is failing when used with a zone override in place. If you call
with a Zone override in place then the correct Zone cwd is returned. If you call
then then the code ignores the override. The problem appears to stem from the fact that Uri.base calls:
_uriBaseClosure then calls
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. I will have a play with your suggested work around, but it still feels like that path has a problem. 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 |
In my opinion Do you have a minimal reproducible example showing that |
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
<#55?email_source=notifications&email_token=AAG32OD3IBFPCJUCSTLTZA3QYQJ5XA5CNFSM4JZFZWY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG3RHII#issuecomment-565646241>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG32OD6HMYT4VTDMZJB6O3QYQJ5XANCNFSM4JZFZWYQ>
.
|
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. Have I understood this correctly? |
I've submitted an issue here: |
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.
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.
The text was updated successfully, but these errors were encountered: