Truncate app name if too long when using ServiceInstanceGuidSuffix
#321
Conversation
Codecov Report
@@ Coverage Diff @@
## master #321 +/- ##
============================================
+ Coverage 86.07% 86.09% +0.02%
- Complexity 983 986 +3
============================================
Files 105 105
Lines 3252 3258 +6
Branches 184 185 +1
============================================
+ Hits 2799 2805 +6
Misses 335 335
Partials 118 118
Continue to review full report at Codecov.
|
| private String assembleNameWithGuidSuffix(String appName, String serviceInstanceId) { | ||
| return appName + CONCAT_DELIM + serviceInstanceId; | ||
| } | ||
|
|
||
| private int calculateAvailableLength(String serviceInstanceId) { | ||
| return CF_SERVICE_NAME_MAX_LENGTH - CONCAT_DELIM.length() - serviceInstanceId.length(); | ||
| } |
There was a problem hiding this comment.
This essentially the same logic as in the production code. I personally tend to avoid using code logic to write test, so that we can validate the behavior in two different styles. The exact value don't actually matter, we just want to make sure the name ends up being unique and is less than 50 characters. So maybe we only need to make sure the name includes service instance id, has length of 50, and, optionally, starts with part of the service name. Wdyt?
There was a problem hiding this comment.
Yep, I see your point. Thanks! We do need to validate that case where the resulting name is less than 50 chars as well. While I don't like duplicating code, I do consider the goal of the test to be to validate the expected results. Even if the code to do that is basically the same now, the new name still needs to be determined somehow for comparison. But I'll reduce that duplication and update the PR.
There was a problem hiding this comment.
Right, it all depends on how we define the goal of this test.
- If the behavior we are trying to describe here is exactly how we are truncating and concatenate names, then it will be hard to avoid duplicating the logic.
- If the behavior is just keep it unique and less than 50 chars, then whether it's a
-in between or how we truncate are not a behavioral requirements that needs to be tested. To achieve this goal, where the resulting name is less than 50 chars, we could assert that it contains the service instance id and the full service name.
There was a problem hiding this comment.
ok, redesigned the tests. thanks.
royclarkson
force-pushed
the
204-truncate-app-name-with-guid-suffix
branch
from
c5b4d88
to
2d94b66
Compare
royclarkson
force-pushed
the
204-truncate-app-name-with-guid-suffix
branch
from
2d94b66
to
647f84e
Compare
Resolves #204