-
Notifications
You must be signed in to change notification settings - Fork 592
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
Remove More Optional Class Testing #2139
Remove More Optional Class Testing #2139
Conversation
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.
It would be good to reenable the cross testing with C#.
See ice/.github/workflows/ci.yml
cpp/test/Ice/logger/log.txt
Outdated
@@ -0,0 +1 @@ | |||
-- 05/08/24 16:59:34115 aplicación: info: XXX |
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.
It looks like this file was added by accident.
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 catch. I was committing too hastily.
Removed.
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.
Maybe we should update the .gitignore
to catch this file if it was generated.
bool supportsRequiredParams(); | ||
|
||
bool supportsJavaSerializable(); | ||
|
||
bool supportsCsharpSerializable(); | ||
|
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.
Should also remove supportsCsharpSerialize above. We dropped support for C# serialization a while ago.
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.
and supportsRequiredParams. I can't find any call to this operation.
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, this is only used by the Java mapping.
Java is the only language which returns true
for this, and it's also the only language that calls it.
At the top of Ice/Optional/AllTests.java
That's part of the Java cleanup I mentioned in the top description.
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.
Looked at the JS and Python and it looked good
This PR follows #2094 in gutting more of the Ice/optional tests.
This is the penultimate PR, since we're there's still some optional classes being passed around via the streaming APIs,
and the Java tests in particular can use some serious cleaning up since 'java:optional' is dead metadata.
It's mostly just removal, so feel free to only look at the languages you care about.
Anyways, this PR:
supportsNullOptional
opMG
tests which test the interplay ofmarshaled-result
with optional classesIt's a simple test, and believe-it-or-not, it's the only test I see that ensures optional data members survive a round trip from client->server->client, which seems worth keeping a test for.