You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by bgould <gi...@git.apache.org> on 2016/01/05 18:28:53 UTC
[GitHub] thrift pull request: THRIFT-3521 : TSimpleJSONProtocolTest is not ...
GitHub user bgould opened a pull request:
https://github.com/apache/thrift/pull/772
THRIFT-3521 : TSimpleJSONProtocolTest is not deterministic (fails when run on JDK 8)
This PR removes fields that cause TSimpleJSONProtocolTest to fail when run using different version of the Java standard library.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bgould/thrift THRIFT-3521
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/772.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #772
----
commit 61efde2a66c3da359c34ef8c377a660e5357b4bd
Author: BCG <bg...@users.noreply.github.com>
Date: 2016-01-05T17:18:23Z
THRIFT-3521 : TSimpleJSONProtocolTest is not deterministic (fails when run on JDK 8)
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3521 : TSimpleJSONProtocolTest is not ...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on the pull request:
https://github.com/apache/thrift/pull/772#issuecomment-169096832
I thought about that too but didn't have time to do it this morning and just wanted to get a working build. I think that perhaps better than a regex might be to actually parse the JSON string and check the values. I'll see if I can get that working.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3521 : TSimpleJSONProtocolTest is not ...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/772#issuecomment-169204403
You're right, you were already keeping those which still work for JDK8.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3521 : TSimpleJSONProtocolTest is not ...
Posted by bgould <gi...@git.apache.org>.
Github user bgould commented on the pull request:
https://github.com/apache/thrift/pull/772#issuecomment-169139570
I see... in that I think that the tests should be ok as they are because there still exist maps and sets in the `testSanePartsOfCompactProtoTestStruct` test method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3521 : TSimpleJSONProtocolTest is not ...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/772#issuecomment-169105689
I was mainly concerned about completely losing map/set tests.
Of course preserving all tests for those element types is great, but I think it's fine to unset most of them, as long as some/one of tests for both map and set is retained.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3521 : TSimpleJSONProtocolTest is not ...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/772#issuecomment-169092979
@bgould, thanks for looking into this.
Would it be tedious and/or complicate the code if we match against regex instead of removing them ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3521 : TSimpleJSONProtocolTest is not ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/772
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---