-
Notifications
You must be signed in to change notification settings - Fork 909
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
os/Chown #4213
os/Chown #4213
Conversation
d1b89f1
to
724b52c
Compare
To design meaningful test cases for
The problem I see with solution 2 is that the implementation of I would appreciate your input on that @rminnich EDIT: Maybe I am mistaken but as I see it, |
@deadprogram do you have an opinion on that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to keep this lightly tested, for example by checking that the syscall returns an expected permission error (or even just "file not found" - it also means the call works at least somewhat).
Yes it would be great to be able to test everything, but that's very hard to do for things like this and you'll end up putting a lot of effort into what's essentially just a wrapper for a libc function.
71d571e
to
8c3e435
Compare
dedff0a
to
b9a32dd
Compare
cebc95d
to
38c58f8
Compare
@leongross can you please rebase this PR against the latest |
@deadprogram rebase done, but noq quite sure where the runtime panic originates. Do you know anything about this? |
@leongross looks like some CI fails on this PR. You probably also need to rebase against latest |
202e583
to
c7e6851
Compare
The old errors should be fixed except for the ci (failing probably due to #4347). Could you review it? @deadprogram |
e10a472
to
0494337
Compare
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
@deadprogram I think this was the last fix now, mind to review? |
Thanks for the additions @leongross and too @rminnich and @aykevl for review. Now squash/merging. |
Add
os.Chown()