remove deprecated service #780
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ewollesen
commented
Oct 4, 2024
data/service/service/client_test.go
Outdated
There was a problem hiding this comment.
This did nothing, so I removed it. I guess it needed to be compiled and run by ginkgo, so... getting rid of it seems like a good thing.
That and the disappointment of reading the client code, then looking for the tests only to find that the file is empty. :(
toddkazakov
previously approved these changes
Oct 5, 2024
|
|
||
| Context("AuthClient", func() { | ||
| It("returns successfully with server token", func() { | ||
| //runtime.Breakpoint() |
…w service The AuthClient test removal is the only thing I find in any way controversial here. That test requests the auth client, then calls a method on it, to ensure that it returns what is expected. However, since the new service doesn't initialize that auth client itself, rather, it's just returning whatever is passed to it via a setter, there's no point in testing this here.
The previous commit did drop one test, because it didn't apply to the new Service. That test does apply to Authenticated however. This commit restores that test, which checks that the configuration passed to the service via the config reporter is able to successfully initialize an authentication client.
I believe there is no loss in test coverage through this action. I manually compared the old tests and new tests to see that they covered the same concepts. Most of them were duplicates to begin with. The one that wasn't duplicated already, was moved to authenticated_test.go in a previous commit. Since the deprecated service is no more, these tests are no longer necessary. They had already be retro-fitted to test the replacement service.Service anyway, where they all passed.
ewollesen
force-pushed
the
eric-platform-deprecated-service
branch
from
31315a1
to
87e2501
Compare
toddkazakov
approved these changes
Oct 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This service was deprecated in 2018. Let's finish the migration.
The data service is the only place that used the deprecated service. I started by expanding the data service to include both a deprecated service and the new service. Then when I had all the tests at equity, I dropped out the old deprecated service.
So all the things that were tested in the deprecated service are still tested (the only thing that wasn't already tested was part of the Authenticated service).
Before this is merged, I'd probably squash it done to a single commit. I'm leaving the separate commits in case someone wants to see the step by step of how I tried to make sure nothing was missed in the migration.