Add support for user defined variables and conditional expressions on CLDTransformation #238
Conversation
… CLDTransformation implemented new classes: CLDVariable CLDExpression CLDConditionExpression CLDOperators Changed CLDTransformation to support the new functionality Added unit tests for higher code coverage
| @@ -0,0 +1,354 @@ | |||
| // | |||
| // CLDTransformationDotNetBaselineTests.swift | |||
There was a problem hiding this comment.
You forgot to change the name of the file and class
There was a problem hiding this comment.
looks like i don't understand your comment, the file contains a literal translation of the DotNet test suit, that the reason for the file name....
There was a problem hiding this comment.
will rename to BaselineTests
| // MARK: - CLDTransformationDotNetBaselineTests | ||
| class CLDTransformationDotNetBaselineTests: BaseTestCase { | ||
|
|
||
| var sut : CLDTransformation! |
There was a problem hiding this comment.
What is the meaning of sut (it's on all the tests)?
There was a problem hiding this comment.
sut - system/subject under test
its a common way of organizing tests, vary helpful to the thinking process
| @discardableResult | ||
| open func setWidth(_ width: String) -> Self { | ||
| return setParam(TransformationParam.WIDTH, value: width) | ||
|
|
There was a problem hiding this comment.
I'm not sure I understand the logic here - why would the value be empty if you just set it? and if it's empty, what's good calling set width with the newly created (empty) expression?
Also, this code repeats in every transformation param that supports variables - if it's indeed necessary, we can extract the logic to a function instead of repeating these 2-3 lines so many times.
There was a problem hiding this comment.
@nitzanj we have an issue with empty expressions that's why the code is needed, in regards to creating a helper function , i'll see what can be done
| @@ -0,0 +1,1106 @@ | |||
| // | |||
There was a problem hiding this comment.
These tests shouldn't be under BaseNetwork/... (this is true for all the test files, not only this one).
There was a problem hiding this comment.
i moved the files
| } | ||
|
|
||
| // MARK: - width | ||
| func test_setWidth_int_shouldReturnValidString() { |
There was a problem hiding this comment.
What gets tested here is the get/set mechanism with the map behind - it's important, but hardly the main issue.
The critical use case is generating the actual transformation string and that is not tested at all. Most of the generation is already tested elsewhere across the SDK, however for next time - it makes more sense to check that when setting a parameter, the string representation of the transformation is generated correctly - that is, in fact, the single purpose of the transformation class.
There was a problem hiding this comment.
@nitzanj we actually wanted to test the setter/getter elements, as we had a issue with them once, the string representation is tested via other scenarios, but if you want me to i can add some explicit tests into this test suit
* master_public: Add support for user defined variables and conditional expressions (cloudinary#238) Update supported swift version in README.md Support array of values for radius (cloudinary#235) Update SPM definitions file (cloudinary#234) # Conflicts: # Example/Tests/BaseNetwork/Core/ResponseTests.swift
* master_private: Add support for user defined variables and conditional expressions (cloudinary#238) # Conflicts: # Cloudinary/Classes/Core/Features/Helpers/CLDTransformation.swift # Example/Cloudinary.xcodeproj/project.pbxproj
* SHA2_UsingCommonCrypto: Add support for user defined variables and conditional expressions (cloudinary#238)
* master_private: Add support for user defined variables and conditional expressions (cloudinary#238) # Conflicts: # Cloudinary/Classes/Core/Features/Helpers/CLDTransformation.swift # Example/Cloudinary.xcodeproj/project.pbxproj
2 participants
Add support for user defined variables and conditional expressions on CLDTransformation
implemented new classes:
CLDVariable
CLDExpression
CLDConditionExpression
CLDOperators
Changed CLDTransformation to support the new functionality
Added unit tests for higher code coverage