Switch 'Ice/optional' Tests to use Optional Structs Instead of Optional Classes

[InsertCreativityHere]

This PR switches some of our testing of optionals to use optional structs, instead of optional classes.

I'd recommend checking the changes made to Test.ice before trying to review the rest of the code, to get an idea of what actually changed.

I'm marking everyone as a reviewer, since this changes lots of language mappings.
Feel free to only review the languages you're familiar with.

[bernardnormier]

We already have extensive testing for struct above, with opSmallStruct, opFixedStryct, opVarStruct and even tests for struct list.

What are these newly renamed operations sendOptionalStruct and returnOptionalStruct adding in terms of test coverage?

It's ok to delete redundant tests.

[InsertCreativityHere]

So, these functions are used to test interop with the 1.0 encoding.

Specifically: optional values are a 1.1-only feature, but we support adding them to operations called with the 1.0 encoding. In this case, the optionals are simply ignored.

This behavior isn't class specific, so using optional structs instead of optional classes is fine. Hence my change.

Note that there isn't really any other operation we could of co-opted for this purpose. Since all the other operations just echo whatever value you pass in. This isn't useful for this test case, since we want to separately ensure that sending and receiving optionals are ignored.

[bernardnormier]

After reading the comment in the .cpp file, I think you missed the intent of this test.

It's not about passing an optional parameter, but about passing a class parameter with optional fields. The additional layer of making the parameter optional is superfluous.

So I would keep the names and simplify the signature:

void sendOneOptionalClass(OneOptional o);
...

[InsertCreativityHere]

If you want to remove it, that's fine with me, I don't really care.
But, I get the opposite idea from reading the original comment:

// Use the 1.0 encoding with operations whose only class parameters are optional.

It doesn't say alot. But it sounds like the point is "... parameters are optional"
and it doesn't mention anything about the fields that class holds.

I tracked down the original issue: https://zeroc.atlassian.net/browse/ICE-4930
I don't really understand what's going on though. It's from 3.4.2 apparently.

[pepone]

Looking at the initial commit that fixes this 493caea

I think this test was about the class encoding, more concretely about a bogus call to writePendingObjects

493caea#diff-a6f653e19cbe0e6ef7bfee702bbb838ccd86ce17d0454083c81e670e4a8020cdL2173-L2176

If we are only sending optional classes seems we don't have to call the method, but be were calling it.

The PR added this additional includeOptional parameter to the Slice::Operation::sendsClasses operation.

I think we can remove the test, as it was class encoding related and the new version doesn't use classes.

[bernardnormier]

G1/G2 and G are apparently used for opG below. It's not clear to me what is special with this test, especially now that G1 and G2 are plain structs.

[InsertCreativityHere]

All of these G types are for testing the combination of optionals and marshaled-result:

    ["marshaled-result"] G opMG1();                     // These are still optional on my
    ["marshaled-result"] G opMG2(G p1, out G p2);       // branch I haven't gotten to them yet.

We have a 4 marshaled-result tests in total.
The first uses optional structs (which hold a single required byte),
the second uses an optional sequence, the third uses an optional dictionary.

But none of these test marshaled-result with optional members.
Hence, I kept the 4th test, since, that's what it tests. A class which holds a combination of optional and required data members. And this test doesn't seem class specific. All we need is a class which holds some optional things internally.

[InsertCreativityHere]

Now, maybe we could remove opG().
It seems we added it as part of a bug-fix: Fixed ICE-6602: optionals marshaling bug, so to play it safe I kept it.
I couldn't find any more detailed explanation in the logs, and forgot how to log into JIRA.

[externl]

LGTM

[ReeceHumphreys]

Just looked at the js, ts, and python to make sure it followed proper practices. Looks good to me!

[InsertCreativityHere]

I removed the tests for sending and returning optional classes with the 1.0 encoding.