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

Remove More Optional Class Testing #2139

Conversation

InsertCreativityHere
Copy link
Member

@InsertCreativityHere InsertCreativityHere commented May 8, 2024

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:

  • removes supportsNullOptional
  • removes our opMG tests which test the interplay of marshaled-result with optional classes
  • removes optional class fields we had stuck in some exceptions
  • simplifies (but keeps) the 'opOneOptional' test.
    It'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.

Copy link
Member

@bernardnormier bernardnormier left a 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

@@ -0,0 +1 @@
-- 05/08/24 16:59:34115 aplicación: info: XXX
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Contributor

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();

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

@InsertCreativityHere InsertCreativityHere May 9, 2024

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.

Copy link
Contributor

@ReeceHumphreys ReeceHumphreys left a 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

@InsertCreativityHere InsertCreativityHere merged commit dc87569 into zeroc-ice:main May 9, 2024
17 checks passed
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