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 TestPermissionDenied should ignore darwin #2561

Merged
merged 1 commit into from
Aug 11, 2021
Merged

Conversation

Abirdcfly
Copy link
Contributor

@Abirdcfly Abirdcfly commented Aug 10, 2021

fix TestPermissionDenied should ignore darwin
Homebrew/homebrew-core#81904 (comment)

Release Note

NONE

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do expect an error there. More exactly a "permission denied" error as we are trying to access /root with no permission for it. We can work on refactoring the whole test and instead creating a real file with root permissions in another directory. The goal of the test is to fail (and not panic) with a permission denied error.

@oscerd
Copy link
Contributor

oscerd commented Aug 10, 2021

Originally i opened that PR. But in the end they were arguing on thing i didn't see on our repo..

@squakez
Copy link
Contributor

squakez commented Aug 10, 2021

Originally i opened that PR. But in the end they were arguing on thing i didn't see on our repo..

Yeah, I think the problem is caused by how the CI is building the application. The best approach is indeed to work on improving the test in order to make it OS agnostic by creating a file and setting some privilege to deny access. I created it some time ago and I did not consider the impact on other systems that can build the application.

I am opening a follow up issue to work on such improvement, in the while, we may even remove/comment the test in order to fix the problem described there.

@Abirdcfly Abirdcfly changed the title fix TestPermissionDenied should use assert.Nil fix TestPermissionDenied should ignore darwin Aug 10, 2021
@Abirdcfly
Copy link
Contributor Author

We do expect an error there. More exactly a "permission denied" error as we are trying to access /root with no permission for it. We can work on refactoring the whole test and instead creating a real file with root permissions in another directory. The goal of the test is to fail (and not panic) with a permission denied error.

Yes, We do expect an error there. the true bug is that when run command go test ./... in darwin, we will get a error like

--- FAIL: TestPermissionDenied (0.00s)
    util_test.go:41:
                Error Trace:    util_test.go:41
                Error:          Expected value not to be nil.
                Test:           TestPermissionDenied

@squakez
Copy link
Contributor

squakez commented Aug 10, 2021

We do expect an error there. More exactly a "permission denied" error as we are trying to access /root with no permission for it. We can work on refactoring the whole test and instead creating a real file with root permissions in another directory. The goal of the test is to fail (and not panic) with a permission denied error.

Yes, We do expect an error there. the true bug is that when run command go test ./... in darwin, we will get a error like

--- FAIL: TestPermissionDenied (0.00s)
    util_test.go:41:
                Error Trace:    util_test.go:41
                Error:          Expected value not to be nil.
                Test:           TestPermissionDenied

I guess that's because Darwin won't return an error when trying to access /root/. I am not entirely sure how the Golang library is behaving when it comes to cross os file managment. I think we can give a try and create a file, setting the permissions, as I suggested in #2563, instead of assuming /root/ is privileged, as we're doing now.

@squakez squakez merged commit 5385f35 into apache:main Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants