-
Notifications
You must be signed in to change notification settings - Fork 829
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
test-refactoring: remove forked class in tests #2845
Conversation
Forked because of #1462 We have now AlphanumericRandomValueStringGenerator and will get the other one. For tests go with AlphanumericRandomValueStringGenerator
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/187469690 The labels on this github issue will be updated when the story is started. |
I'm having trouble understanding the context. The code changes are easy enough to review on the surface. @strehle could you explain the motivation? Why were some RandomValueStringGenerator uses replaced but not all? |
The productive usage of this class is in context of OAuth2 and this stays. This PR is only for test usages: History: This new chars caused many test failures in UAA because we used the random for client / user creation.... and many test scenarios. Therefore I forked the original class from ext. project into UAA test scope, see Later there was the new class https://github.com/cloudfoundry/uaa/blob/develop/server/src/main/java/org/cloudfoundry/identity/uaa/util/AlphanumericRandomValueStringGenerator.java because others in UAA had similar issues. Now (back in present) we want remove the spring security oauth2 dependency but the RandomValueStringGenerator still is in test, therefore this PR is a cleanup to remove this fork and re-use the AlphanumericRandomValueStringGenerator in places where we need random without special chars. You may see that all changes are only in TEST code, therefore this extra cleanup PR because I see it as a easy to approve PR and thought its better to have it in a extra PR and not within #2813 |
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.
Thanks for splitting out this PR.
Forked because of
#1462
We have now AlphanumericRandomValueStringGenerator and will get the other one.
For tests go with AlphanumericRandomValueStringGenerator
Prevent problems with #2813 where we now have the same class also in UAA context