Skip to content

Adding junits for JsonToStringStyle#391

Merged
chtompki merged 2 commits intoapache:masterfrom
RahulNagekar:JsonToStringStyle_Junits
Feb 18, 2019
Merged

Adding junits for JsonToStringStyle#391
chtompki merged 2 commits intoapache:masterfrom
RahulNagekar:JsonToStringStyle_Junits

Conversation

@RahulNagekar
Copy link
Copy Markdown
Contributor

No description provided.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 2, 2018

Coverage Status

Coverage increased (+0.3%) to 95.544% when pulling cee7cc6 on RahulNagekar:JsonToStringStyle_Junits into e0a5a4b on apache:master.

@RahulNagekar RahulNagekar force-pushed the JsonToStringStyle_Junits branch from 59ef8c8 to 77bd11f Compare December 2, 2018 21:08
@RahulNagekar RahulNagekar reopened this Dec 2, 2018
@RahulNagekar RahulNagekar force-pushed the JsonToStringStyle_Junits branch 2 times, most recently from 97ce7f8 to 5edde33 Compare December 4, 2018 21:02
@RahulNagekar
Copy link
Copy Markdown
Contributor Author

This is ready for merge , could we please have this handled.

@kinow
Copy link
Copy Markdown
Member

kinow commented Dec 13, 2018

This project is maintained by volunteers @RahulNagekar. Please be patient as maintainers can be busy or under pressure with other issues, work, life, etc.

@MarkDacek
Copy link
Copy Markdown
Contributor

Can you make your arrays in the new test cases final? Also perhaps avoid using the new keyword when you don't need memory allocated at that exact moment.

Paging @chtompki for context. Should the JSON blurbs that are the expected output be final Strings as well? That's not the existing pattern, but it seems a bit odd to be concatenating to make deterministic Strings during an assert.

@chtompki
Copy link
Copy Markdown
Member

I'm fairly indifferent about how the strings are declared as they are tests. The concatenation is a tad odd to me because it seems to be unnecessary. That said, I'm all for more tests. I say we pull this in and clean it up in master because having more tests is always a good thing.

@chtompki
Copy link
Copy Markdown
Member

I'll try to get to this today.

@RahulNagekar
Copy link
Copy Markdown
Contributor Author

Will post 1 more commit improving this. I am all agree with all comnents , its best to follow best practices.

@RahulNagekar RahulNagekar force-pushed the JsonToStringStyle_Junits branch from 5edde33 to cee7cc6 Compare December 20, 2018 19:39
@MarkDacek
Copy link
Copy Markdown
Contributor

@chtompki This seems reasonable and passes both verify and checkstyle. Worth pulling in?

@chtompki chtompki merged commit 589ce59 into apache:master Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants