search

cli / go-gh

A Go module for interacting with gh and the GitHub API from the command line.
https://pkg.go.dev/github.com/cli/go-gh/v2
MIT License
322 stars 45 forks source link

Treat tenancy as non enterprise #151

Closed williammartin closed 3 months ago

williammartin commented 4 months ago

Description

I think there's some things I'd like to move from https://github.com/cli/cli/blob/24481c5c2e49d3529a6621953b8878fe0514ff22/internal/ghinstance/host.go into go-gh but I think having this behaviour as an obvious standalone change is valuable without muddying the waters.

williammartin commented 4 months ago

Would we want to bring over the isTenancy() tests over too?

Not specifically but this reminded me that I definitely want to add tests to https://github.com/cli/go-gh/blob/d88d88f917cd4e0bcaa582bac2bb909bd79f9759/pkg/auth/auth_test.go#L10

Really silly of me to forget.

andyfeller commented 4 months ago

Would we want to bring over the isTenancy() tests over too?

Not specifically but this reminded me that I definitely want to add tests to

https://github.com/cli/go-gh/blob/d88d88f917cd4e0bcaa582bac2bb909bd79f9759/pkg/auth/auth_test.go#L10

Really silly of me to forget.

I know, but I hate to say we have a test suite for IsEnterprise already:

https://github.com/cli/go-gh/blob/ee82f3a57020e59e18befbb7f39b5aae62cfeb41/pkg/auth/auth_test.go#L229-L258

You really sure?

williammartin commented 4 months ago

Bleh. So firstly, I have to say that the existing tests are already too knowledgable about implementation details. That the public function TokenForHost is totally untested is an indictment on the current state of these tests.

image

TokenForHost is what users of the go-gh module rely on. That's the function that they expect to work. Testing the private functions that support TokenForHost instead not only reduces our ability to avoid regressions but actually congeals the existing implementation, increasing maintenance costs.

I'd added cases to the private tokenForHost test try and get as much of a black box test as possible within the current framework. At your prompting I've also added a test to communicate intent around how isEnterprise should handle tenants. I don't think there's much value in adding tests around isTenant.

I know, but I hate to say we have a test suite for IsEnterprise already

Yeh and I don't think it's good. What I really want to do is rework all the tests to avoid them knowing about any private functions but I don't have the appetite to tackle that here.

All that said, I really appreciate you pushing on the tests angle 😊