Skip to content
This repository was archived by the owner on Jan 31, 2020. It is now read-only.

Fix typos in DirWriteable check + add unit test based on vfsStream #65

Merged
merged 4 commits into from
Mar 3, 2016
Merged

Conversation

Headd2k
Copy link
Contributor

@Headd2k Headd2k commented Feb 12, 2016

Hi!

First of all i want to say "thank you" for these components! We use them in production together with the Liip Monitor bundle and its very very helpful. So: Thanks for your work!

I recently spotted a small typo in the "DirWritable" check. There was an doubled "a" in the message "The path is a writable directory." Additionally i added some missing dots. :)

I tried to fix it with the existing unit-tests but unfortunately they do not work for me mainly because i'm on a Windows system (chmod stuff simply doesnt work on win machines). So i decided to add a new testsuite especially for the DirWriteable check implementing checks using a virtual filesystem based on vfsStream.

Actually i dont know if this pr breaks some of the existing tests. There is no need to add my tests when you fix the typo by yourself. :)

Thanks again.

Regards

Kai

@Thinkscape
Copy link
Member

Thanks @Headd2k.

Is there any advantage of using vfs for those tests? I feel like adding dependency on that makes it more fragile. This is in the context of posix system (and fs) which allows chmods.

I was considering suggesting writing a different windows test case where you'd use windows ACL tools to modify permissions (instead of chmod)... the thing is, it might fail in case someone run the tests on i.e. FAT32 filesystem.

On the other hand, the vfs dependency might in future give false positives on real posix systems. The least we should do is make vfs tests run on windows only.

What are your thoughts?

@Headd2k
Copy link
Contributor Author

Headd2k commented Feb 27, 2016

hi @Thinkscape

thanks for your reply!

to be honest: i actually have no feelings pro or contra vfs. :)

i thought it might be the better way to test fs related stuff. i use vfs in my tests since a couple of years. the current directory related tests failed on my windows machine when chmod comes in place so i've set up vfs to test my changes.

to be clear: i just develop on windows but have all my production envionments on ubuntu.

you can delete this pr if you feel it might not be the right way for this project. i just had to fill a lazy afternoon. :)

regards

kai

@Thinkscape
Copy link
Member

I don't really want to delete anything :-)

What I'm considering is if we should use VFS on linux, where it's not really necessary.

@Headd2k
Copy link
Contributor Author

Headd2k commented Mar 2, 2016

@Thinkscape :)

the vfs is only a dependency for dev environments where you want to have the ability to run the tests. so i actually dont see this making the project "fragile" in any way. the tests using the real fs are currently still in place and executed during ci so it might be a "win" for both of us. :)

again: i have no big feelings about this pr. so it is really okay to refuse it. really! :)

maybe this little info page will help you on your decision: https://phpunit.de/manual/current/en/test-doubles.html#test-doubles.mocking-the-filesystem

regards

kai

Thinkscape added a commit that referenced this pull request Mar 3, 2016
Fix typos in DirWriteable result descriptions, add DirWritable unit tests.
@Thinkscape Thinkscape merged commit 8e277c6 into zendframework:master Mar 3, 2016
@Thinkscape
Copy link
Member

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants