-
Notifications
You must be signed in to change notification settings - Fork 393
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
improved coverage for JsonUtils #73
improved coverage for JsonUtils #73
Conversation
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Sravanthi Konduru <s***@s***.i***.s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce.com Contributor License Agreement and this Pull Request will be revalidated. |
3df1a8f
to
5f60077
Compare
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
+ Coverage 85.88% 85.97% +0.08%
==========================================
Files 294 294
Lines 9530 9530
Branches 320 535 +215
==========================================
+ Hits 8185 8193 +8
+ Misses 1345 1337 -8
Continue to review full report at Codecov.
|
Thanks for the contribution! It looks like @sravanthi-konduru is an internal user so signing the CLA is not required. However, we need to confirm this. |
@sravanthi-konduru I've invited you to: https://github.com/salesforce Once accepted, refresh the status of the CLA: https://cla.salesforce.com/status/salesforce/TransmogrifAI/pull/73 |
|
||
case class Address(city: String, state: String) | ||
|
||
private object Format extends Enumeration { |
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.
No need to reate enum. Simple create two check function: checkYaml and checkJson
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.
Done!
forAll(doubles) { d => check(TestDouble(d, Array.empty, Seq.empty, Map.empty, None), Format.Yaml) } | ||
} | ||
|
||
property(testName = "handle json file reading properly") { |
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.
Simply do: property(“read json file”)
def check(data: TestDouble): Unit = { | ||
val json = JsonUtils.toJsonString(data) | ||
JsonUtils.fromString[TestDouble](json) match { | ||
property(testName = "handle yml file reading properly") { |
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.
property(“read yaml file”)
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.
Done!
utils/src/test/resources/Person.json
Outdated
@@ -0,0 +1,7 @@ | |||
{ | |||
"name": "Alvin Alexander", |
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.
Is this a random name? Lets just put name “Foo Bar”
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.
Yes, it is random. Updated to Foo Bar!
val person = Person("Foo Bar", Address("Talkeetna", "AK")) | ||
JsonUtils.fromFile[Person](ymlFile) match { | ||
case Failure(e) => fail(e) | ||
case Success(value) => assert(value, person) |
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.
since it's a case class you dont need a special assertion method but you can use the equality directly, i.e value shouldBe person
- http://www.alessandrolacava.com/blog/scala-case-classes-in-depth/
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.
Good to know that.Thanks. Updated!
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.
one las comment left, otherwise lgtm
…nduru/TransmogrifAI into unittest/jsonutils-coverage
Improve code coverage for JsonUtils
Related issues
N/A
Describe the proposed solution
Added tests for yaml mapper and file parsing
Describe alternatives you've considered
N/A
Additional context
N/A