Skip to content
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

Add new unit tests for JsonParser; implement getContactEmails in Dataverse #2795

Closed
wants to merge 1 commit into from

Conversation

bencomp
Copy link
Contributor

@bencomp bencomp commented Dec 4, 2015

Rebased on the 4.3 branch, thinking that you should enough time to review the PR :)

Although I promised to not spend more free time on coding for Dataverse, I couldn't
help myself after interacting with the "native API" and talking about code coverage
with @pdurbin some more.

The new tests parse JSON test files from the test resources - rather than test that
JsonPrinter output can be parsed. Only date and date-time tests test a round-trip.
I added comments to explain what the test tries to accomplish.

Although line coverage of JsonParser with these tests is > 56%, not all branches are
covered even though NetBeans may show green: inline if/else statements 'hide' some
branches in the green lines.

To make the tests pass, I added dv.setAffiliation in JsonParser.parseDataverse
with a default value of null if the JSON doesn't include affiliation and
implemented Dataverse.getContactEmails to produce a comma-separated string of
email addresses. I hope these have been good assumptions for how the API behaves.

Note that because the JsonParser works on a different level than the API, the default
values that the parser uses may be different than the business logic prescribes
(see dv.setPermissionRoot in parseDataverse). The tests expect the parser's
defaults.

…verse

Although I promised to not spend more free time on coding for Dataverse, I couldn't
help myself after interacting with the "native API" and talking about code coverage
with @pdurbin some more.

The new tests parse JSON test files from the test resources - rather than test that
JsonPrinter output can be parsed. Only date and date-time tests test a round-trip.
I added comments to explain what the test tries to accomplish.

Although line coverage of JsonParser with these tests is > 56%, not all branches are
covered even though NetBeans may show green: inline if/else statements 'hide' some
branches in the green lines.

To make the tests pass, I added `dv.setAffiliation` in `JsonParser.parseDataverse`
with a default value of `null` if the JSON doesn't include affiliation and
implemented `Dataverse.getContactEmails` to produce a comma-separated string of
email addresses. I hope these have been good assumptions for how the API behaves.

Note that because the JsonParser works on a different level than the API, the default
values that the parser uses may be different than the business logic prescribes
(see `dv.setPermissionRoot` in `parseDataverse`). The tests expect the parser's
defaults.

Removed unused imports in JsonParserTest.
@bencomp
Copy link
Contributor Author

bencomp commented Dec 4, 2015

This is the renewed #2478.

@pdurbin pdurbin added this to the 4.3 milestone Jan 5, 2016
@pdurbin
Copy link
Member

pdurbin commented Jan 8, 2016

@scolapasta a comment from @michbarsinai - "The whole getContactEmails() thing is, I think, on its way out, and was never really used (currently it's hard coded to "")."

He said you should take a look.

@pdurbin pdurbin added the Component: Code Infrastructure formerly "Feature: Code Infrastructure" label Jan 8, 2016
@michbarsinai
Copy link
Member

That is indeed my comment, except that I'm cooking for, like, 12 people and so @pdurbin beat me to commenting it.

Sent from my iPhone

On 8 בינו׳ 2016, at 16:25, Philip Durbin notifications@github.com wrote:

@scolapasta a comment from @michbarsinai - "The whole getContactEmails() thing is, I think, on its way out, and was never really used (currently it's hard coded to "")."


Reply to this email directly or view it on GitHub.

@pdurbin
Copy link
Member

pdurbin commented Jan 8, 2016

"My name is @michbarsinai and I approve this message." #election2016 ;)

@pdurbin pdurbin closed this Jan 13, 2016
@pdurbin
Copy link
Member

pdurbin commented Jan 13, 2016

@bencomp please see my comments about closing pull requests made against the 4.3 branch at #2860 (comment) (Let's discuss in one place, there.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Code Infrastructure formerly "Feature: Code Infrastructure"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants