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

Improving opt test to include missing values #54

Closed
wants to merge 1 commit into from

Conversation

quidryan
Copy link

@quidryan quidryan commented Aug 9, 2016

@stleary
Copy link
Owner

stleary commented Aug 9, 2016

Will consider after the revert is completed. Thanks again for catching it.

@quidryan
Copy link
Author

quidryan commented Aug 9, 2016

I wasn't following the previous work that had to be reverted, but wouldn't this test be relevant irrelevant to the state of the JSON-Java repo? This add coverage which did not exist before.

@stleary
Copy link
Owner

stleary commented Aug 9, 2016

Well, the tests are not even included in the Maven repo. We keep them release-synced, so there is always a test release to match the JSON release, but there is no requirement for them to be timestamp-synced, if we are rushed. Since @johnjaylward has also made a pull request in #55, we will proceed there. Please take a look at that branch and post if you see any tests that should be added.

@stleary stleary closed this Aug 9, 2016
@johnjaylward
Copy link
Contributor

@quidryan , I basically did the same as you, but extended the tests to cover more opt methods in #55

I believe my PR has more coverage than your even though we took the same approach. Let me know if you see anything else that needs to be added.

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.

3 participants