Fixes 2896: refactor pulp client in dao #438
Conversation
rverdile
force-pushed
the
pc-3
branch
3 times, most recently
from
8701643 to
e553803
Compare
rverdile
changed the title
pkg/dao/snapshots_test.go
Outdated
| mockPulpClient.On("WithContext", context.Background()).Return(mockPulpClient).Twice() | ||
| mockPulpClient.On("GetContentPath").Return(testContentPath, nil).Twice() | ||
| mockPulpClient.On("WithDomain", "").Return(mockPulpClient) |
There was a problem hiding this comment.
I think this PR is a step in the right direction, although it feels like it adds a lot more boiler plate (like this) to the tests which is not ideal.
There was a problem hiding this comment.
here's an idea (if you think it makes sense) . We could add some methods to the mock that do this for us. I imagine something like:
mockPulpClient.WithContext().WithDomain().
On("GetContentPath").Return(testContentPath, nil).Twice()
and under the hood, this would mock WithContext & WithDomain MinTimes(1) so we'd lose the count on those particular mock calls, but I'm not sure we actually care for that to be precise. I would imagine these methods would have to live in some new file like "pulp_client_mock_helpers.go" or something?
There was a problem hiding this comment.
actually i realized MinTimes() is in go mock and not testify. It looks like if you just don't specify Times() it doesn't care the number
There was a problem hiding this comment.
I like this idea. I'll make that change.
|
this needs a rebase now too |
rverdile
force-pushed
the
pc-3
branch
3 times, most recently
from
d699fab to
08bc4e6
Compare
pkg/dao/repository_configs.go
Outdated
| return nil | ||
| func (r *repositoryConfigDaoImpl) WithContext(ctx context.Context) RepositoryConfigDao { | ||
| r.ctx = ctx | ||
| return r |
There was a problem hiding this comment.
Should this return a copy of the DoaImpl instead of the same DaoImpl?
I'm imagining two requests coming in at the same time, being run at the same time in the same process. They are sharing a DaoImpl, so if they each call .WithContext, they could stomp over each other?
Its very possible i'm not understanding something corretly thougH :)
There was a problem hiding this comment.
I think you're right. I'll see if I can get that to return a copy such that mocking still works in the tests.
|
/retest |
|
this is updated now |
|
/retest |
1 similar comment
|
/retest |
Summary
The method I introduced here to set the domain scoped pulp client is bug-prone.
This PR demonstrates a new way of setting the pulp client by adding two chained methods. Now, we can initialize the pulp client with
context.Background()during dependency injection. Then, replace the context later, during the request, so that the pulp client is using the domain.For example, the pulp client method
GetContentPath()is called withpulpClient.WithContext(ctx).WithDomain(domainName).GetContentPath()I've commented out most of the failing tests for now, as I was just trying out this approach.