add tests for Data Model coverage #218
Conversation
jaeopt
changed the title
yasirfolio3
force-pushed
the
jae/coverDataModel
branch
from
a5ba4e5 to
57ba40a
Compare
| @@ -81,10 +81,13 @@ enum AttributeValue: Codable, Equatable, CustomStringConvertible { | |||
| return | |||
| } | |||
|
|
|||
| if let value = try? container.decode(Int64.self) { | |||
| // Int values are all filtered as Double, so this Int parsing is not needed here | |||
There was a problem hiding this comment.
remove the comments and commented out code.
| @@ -95,9 +95,6 @@ extension Array where Element == ConditionHolder { | |||
| } | |||
|
|
|||
| func evaluate(op: LogicalOp, project: ProjectProtocol?, attributes: OptimizelyAttributes?) throws -> Bool { | |||
| guard self.count > 0 else { | |||
There was a problem hiding this comment.
why is this removed?
There was a problem hiding this comment.
This condition is covered by the caller, so the guard-else condition never happens
There was a problem hiding this comment.
Since the statement below will cause the app to blow up and you can easily add a test case that would cover this for code coverage, I'm going to ask that you put it back in there. The reason being that if for any reason you called evaluate on a array with one element, it will throw an exception as it exists now.
| @@ -97,7 +97,7 @@ extension UserAttribute { | |||
|
|
|||
| // invalid match - parsed for forward compatibility only (but evaluation fails) | |||
| guard let matchFinal = matchSupported else { | |||
| throw OptimizelyError.userAttributeInvalidMatch(self.match ?? "empty") | |||
| throw OptimizelyError.userAttributeInvalidMatch(self.match!) | |||
There was a problem hiding this comment.
there is no guarantee that match is not nil. so either bind it or default it but not bang.
There was a problem hiding this comment.
nil match case is filtered out by "matchSupported", so safe for bang to remove unused code.
Will refactor the code to make safety more clear.
| override public var description: String { | ||
| return "[url] \(url) (" + (String(data: body, encoding: .utf8) ?? "UNKNOWN") + ")" | ||
| } | ||
|
|
There was a problem hiding this comment.
why did you move this?
There was a problem hiding this comment.
Looks like redundant to "isEqual" above. WIl be removed.
| @@ -0,0 +1,23 @@ | |||
| /**************************************************************************** | |||
There was a problem hiding this comment.
why is this class here?
There was a problem hiding this comment.
Good catch. Looks like an error introduced while merging. Will clean up xcode project.
| @@ -95,9 +95,6 @@ extension Array where Element == ConditionHolder { | |||
| } | |||
|
|
|||
| func evaluate(op: LogicalOp, project: ProjectProtocol?, attributes: OptimizelyAttributes?) throws -> Bool { | |||
| guard self.count > 0 else { | |||
There was a problem hiding this comment.
Since the statement below will cause the app to blow up and you can easily add a test case that would cover this for code coverage, I'm going to ask that you put it back in there. The reason being that if for any reason you called evaluate on a array with one element, it will throw an exception as it exists now.
| } | ||
| } | ||
|
|
||
| func testEvaluate_OperatorOnly() { |
There was a problem hiding this comment.
Where is the error thrown here? This appears to now hit the self[1...] where it would throw an exception
There was a problem hiding this comment.
I guess you expect out-of-bound exception here. It does not happen since the array size is 1 here ( "and" is not filtered out yet).
No description provided.