Skip to content
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

Fix File.valid() and clarify File behavior in error cases #2656

Merged
merged 8 commits into from
May 2, 2018

Conversation

mfelsche
Copy link
Contributor

mostly by writing tests for these cases.

TL;DR the behavior is as expected in error cases.

fixes #2570

mostly by writing tests for these cases
@mfelsche mfelsche added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Apr 16, 2018
@mfelsche
Copy link
Contributor Author

The tests fail due to:

1.) It seems file modes do not apply for circle ci mounted volumes
2.) I am not 100% sure about the FS implementation of appveyor, but the same seem to apply here.

@mfelsche
Copy link
Contributor Author

The actual issue seems to be having the tests being run as root for whom file modes do not apply. Would anyone mind me changing the ci docker images to be run as a non-root user?

@SeanTAllen
Copy link
Member

Do other tests rely on being root? If not, I don't mind.

@jemc
Copy link
Member

jemc commented Apr 18, 2018

If we do that, we should probably also find a way to issue a warning when someone tries to run the tests as root.

Mentioning the circleci cli for running CI jobs locally.
@mfelsche
Copy link
Contributor Author

According to the windows _chmod docs all files are always readable. chmod like fine grained control can only be gained by using ACLs: https://msdn.microsoft.com/en-us/library/windows/desktop/aa379283(v=vs.85).aspx

I will adapt the tests for windows accordingly (and add some comments about the file readability on windows).

@SeanTAllen
Copy link
Member

@mfelsche is this good to merge now? it looks good to us. if you worked out the root issues, then we all think you should hit the big green button that does the merging.

@mfelsche
Copy link
Contributor Author

mfelsche commented May 2, 2018

Thanks for the heads up. The root issues are running stuff as root on linux, which ignores filesystem permissions (fixed) and windows filesystem weirdness (acounted for).

Merging...

@mfelsche mfelsche merged commit c4ebe6d into master May 2, 2018
ponylang-main added a commit that referenced this pull request May 2, 2018
dipinhora pushed a commit to dipinhora/ponyc that referenced this pull request Jun 5, 2018
)

* Fix File.valid() and clarify File behavior in error cases

mostly by writing tests for these cases

* ensure dir is deletable and deleted after test

* try fix chmod errors on circleci

* run ci docker images as non-root user pony

and adapt circleci config to run as non-root user

* use adduser on alpine to create pony user in ci docker images

* update ci dockerfile READMEs

Mentioning the circleci cli for running CI jobs locally.

* warn if tests relying on filesystem permissions are run as root

* add windows exceptions to file permissions for files tests
dipinhora pushed a commit to dipinhora/ponyc that referenced this pull request Jun 5, 2018
@jemc jemc deleted the files-tests branch January 8, 2019 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing tests and unclear behaviour of the File class
3 participants