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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
May 1, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7d3c7c1
Switch some tests to use optional structs instead of optional classes.
InsertCreativityHere Apr 30, 2024
9947059
First round of fixes to other languages
InsertCreativityHere Apr 30, 2024
356a13e
Hopefully the last of them.
InsertCreativityHere Apr 30, 2024
11ae20f
Remove send/return optional test functions.
InsertCreativityHere May 1, 2024
1468b4c
We were reusing a variable later on in a test...
InsertCreativityHere May 1, 2024
8725b8d
More variable re-use.
InsertCreativityHere May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 30 additions & 31 deletions cpp/test/Ice/optional/AllTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,11 @@ class FObjectReader : public Ice::Value
_f = make_shared<F>();
in->startValue();
in->startSlice();
// Don't read af on purpose
// in.read(1, _f->af);
// Don't read fsf on purpose
// in.read(1, _f->fsf);
in->endSlice();
in->startSlice();
in->read(_f->ae);
in->read(_f->fse);
in->endSlice();
in->endValue();
}
Expand Down Expand Up @@ -645,27 +645,27 @@ allTests(Test::TestHelper* helper, bool)
}

//
// Use the 1.0 encoding with operations whose only class parameters are optional.
// Test that optional parameters are handled correctly (ignored) with the 1.0 encoding.
//
optional<OneOptionalPtr> oo(make_shared<OneOptional>(53));
initial->sendOptionalClass(true, oo);
initial->ice_encodingVersion(Ice::Encoding_1_0)->sendOptionalClass(true, oo);
optional<FixedStruct> ofs{{53}};
initial->sendOptionalStruct(true, ofs);
initial->ice_encodingVersion(Ice::Encoding_1_0)->sendOptionalStruct(true, ofs);

initial->returnOptionalClass(true, oo);
test(oo);
initial->ice_encodingVersion(Ice::Encoding_1_0)->returnOptionalClass(true, oo);
test(!oo);
initial->returnOptionalStruct(true, ofs);
test(ofs);
initial->ice_encodingVersion(Ice::Encoding_1_0)->returnOptionalStruct(true, ofs);
test(!ofs);

GPtr g = make_shared<G>();
g->gg1Opt = make_shared<G1>("gg1Opt");
g->gg2 = make_shared<G2>(10);
g->gg2Opt = make_shared<G2>(20);
g->gg1 = make_shared<G1>("gg1");
g->gg1Opt = G1{"gg1Opt"};
g->gg2 = G2{10};
g->gg2Opt = G2{20};
g->gg1 = G1{"gg1"};
GPtr r = initial->opG(g);
test("gg1Opt" == r->gg1Opt.value()->a);
test(10 == r->gg2->a);
test(20 == r->gg2Opt.value()->a);
test("gg1" == r->gg1->a);
test("gg1Opt" == r->gg1Opt.value().a);
test(10 == r->gg2.a);
test(20 == r->gg2Opt.value().a);
test("gg1" == r->gg1.a);

initial->opVoid();

Expand Down Expand Up @@ -765,15 +765,15 @@ allTests(Test::TestHelper* helper, bool)

cout << "ok" << endl;

cout << "testing marshaling of objects with optional objects..." << flush;
cout << "testing marshaling of objects with optional members..." << flush;
{
FPtr f = make_shared<F>();

f->af = make_shared<A>();
f->ae = *f->af;
f->fsf = FixedStruct{56};
f->fse = *f->fsf;

FPtr rf = dynamic_pointer_cast<F>(initial->pingPong(f));
test(rf->ae == *rf->af);
test(rf->fse == *rf->fsf);

factory->setEnabled(true);
Ice::OutputStream out(communicator);
Expand All @@ -789,7 +789,7 @@ allTests(Test::TestHelper* helper, bool)
factory->setEnabled(false);

rf = dynamic_cast<FObjectReader*>(obj.get())->getF();
test(rf->ae && !rf->af);
test(rf->fse.m == 56 && !rf->fsf);
}
cout << "ok" << endl;

Expand Down Expand Up @@ -1294,23 +1294,22 @@ allTests(Test::TestHelper* helper, bool)

{
FPtr f = make_shared<F>();
f->af = make_shared<A>();
(*f->af)->requiredA = 56;
f->ae = *f->af;
f->fsf = FixedStruct();
(*f->fsf).m = 56;
f->fse = *f->fsf;

Ice::OutputStream out(communicator);
out.startEncapsulation();
out.write(1, make_optional(f));
out.write(2, make_optional(f->ae));
out.write(2, make_optional(f->fse));
out.endEncapsulation();
out.finished(inEncaps);

Ice::InputStream in(communicator, out.getEncoding(), inEncaps);
in.startEncapsulation();
optional<APtr> a;
in.read(2, a);
in.read(2, ofs);
in.endEncapsulation();
test(a && *a && (*a)->requiredA == 56);
test(ofs && (*ofs).m == 56);
}
cout << "ok" << endl;

Expand Down
12 changes: 6 additions & 6 deletions cpp/test/Ice/optional/Test.ice
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,20 +161,20 @@ class OptionalWithCustom

class E
{
A ae;
FixedStruct fse;
}

class F extends E
{
optional(1) A af;
optional(1) FixedStruct fsf;
}

class G1
struct G1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@InsertCreativityHere InsertCreativityHere Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
string a;
}

class G2
struct G2
{
long a;
}
Expand Down Expand Up @@ -278,9 +278,9 @@ interface Initial

void opClassAndUnknownOptional(A p);

void sendOptionalClass(bool req, optional(1) OneOptional o);
void sendOptionalStruct(bool req, optional(1) FixedStruct ofs);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@pepone pepone May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


void returnOptionalClass(bool req, out optional(1) OneOptional o);
void returnOptionalStruct(bool req, out optional(1) FixedStruct ofs);

G opG(G g);

Expand Down
12 changes: 6 additions & 6 deletions cpp/test/Ice/optional/TestAMD.ice
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,20 +161,20 @@ class OptionalWithCustom

class E
{
A ae;
FixedStruct fse;
}

class F extends E
{
optional(1) A af;
optional(1) FixedStruct fsf;
}

class G1
struct G1
{
string a;
}

class G2
struct G2
{
long a;
}
Expand Down Expand Up @@ -278,9 +278,9 @@ interface Initial

void opClassAndUnknownOptional(A p);

void sendOptionalClass(bool req, optional(1) OneOptional o);
void sendOptionalStruct(bool req, optional(1) FixedStruct ofs);

void returnOptionalClass(bool req, out optional(1) OneOptional o);
void returnOptionalStruct(bool req, out optional(1) FixedStruct ofs);

G opG(G g);

Expand Down
10 changes: 5 additions & 5 deletions cpp/test/Ice/optional/TestAMDI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,9 +409,9 @@ InitialI::opClassAndUnknownOptionalAsync(
}

void
InitialI::sendOptionalClassAsync(
InitialI::sendOptionalStructAsync(
bool,
optional<shared_ptr<Test::OneOptional>>,
optional<Test::FixedStruct>,
function<void()> response,
function<void(exception_ptr)>,
const Ice::Current&)
Expand All @@ -420,13 +420,13 @@ InitialI::sendOptionalClassAsync(
}

void
InitialI::returnOptionalClassAsync(
InitialI::returnOptionalStructAsync(
bool,
function<void(const optional<shared_ptr<Test::OneOptional>>&)> response,
function<void(const optional<Test::FixedStruct>&)> response,
function<void(exception_ptr)>,
const Ice::Current&)
{
response(make_shared<OneOptional>(53));
response(Test::FixedStruct{53});
}

void
Expand Down
8 changes: 4 additions & 4 deletions cpp/test/Ice/optional/TestAMDI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,16 +261,16 @@ class InitialI final : public Test::Initial
std::function<void(std::exception_ptr)>,
const Ice::Current&) final;

void sendOptionalClassAsync(
void sendOptionalStructAsync(
bool,
std::optional<std::shared_ptr<Test::OneOptional>>,
std::optional<Test::FixedStruct>,
std::function<void()>,
std::function<void(std::exception_ptr)>,
const Ice::Current&) final;

void returnOptionalClassAsync(
void returnOptionalStructAsync(
bool,
std::function<void(const std::optional<std::shared_ptr<Test::OneOptional>>&)>,
std::function<void(const std::optional<Test::FixedStruct>&)>,
std::function<void(std::exception_ptr)>,
const Ice::Current&) final;

Expand Down
6 changes: 3 additions & 3 deletions cpp/test/Ice/optional/TestI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,14 +340,14 @@ InitialI::opClassAndUnknownOptional(APtr, const Ice::Current&)
}

void
InitialI::sendOptionalClass(bool, optional<OneOptionalPtr>, const Ice::Current&)
InitialI::sendOptionalStruct(bool, optional<FixedStruct>, const Ice::Current&)
{
}

void
InitialI::returnOptionalClass(bool, optional<OneOptionalPtr>& o, const Ice::Current&)
InitialI::returnOptionalStruct(bool, optional<FixedStruct>& ofs, const Ice::Current&)
{
o = make_shared<OneOptional>(53);
ofs = FixedStruct{53};
}

GPtr
Expand Down
4 changes: 2 additions & 2 deletions cpp/test/Ice/optional/TestI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ class InitialI : public Test::Initial

virtual void opClassAndUnknownOptional(Test::APtr, const Ice::Current&);

virtual void sendOptionalClass(bool, std::optional<Test::OneOptionalPtr>, const Ice::Current&);
virtual void sendOptionalStruct(bool, std::optional<Test::FixedStruct>, const Ice::Current&);

virtual void returnOptionalClass(bool, std::optional<Test::OneOptionalPtr>&, const Ice::Current&);
virtual void returnOptionalStruct(bool, std::optional<Test::FixedStruct>&, const Ice::Current&);

virtual ::Test::GPtr opG(::Test::GPtr g, const Ice::Current&);

Expand Down
53 changes: 25 additions & 28 deletions csharp/test/Ice/optional/AllTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,17 +337,17 @@ public class AllTests : global::Test.AllTests
factory.setEnabled(false);

//
// Use the 1.0 encoding with operations whose only class parameters are optional.
// Test that optional parameters are handled correctly (ignored) with the 1.0 encoding.
//
var oo = new Ice.Optional<Test.OneOptional>(new Test.OneOptional(53));
initial.sendOptionalClass(true, oo);
var ofs = new Ice.Optional<Test.FixedStruct>(new Test.FixedStruct(53));
initial.sendOptionalStruct(true, ofs);
Test.InitialPrx initial2 = (Test.InitialPrx)initial.ice_encodingVersion(Ice.Util.Encoding_1_0);
initial2.sendOptionalClass(true, oo);
initial2.sendOptionalStruct(true, ofs);

initial.returnOptionalClass(true, out oo);
test(oo.HasValue);
initial2.returnOptionalClass(true, out oo);
test(!oo.HasValue);
initial.returnOptionalStruct(true, out ofs);
test(ofs.HasValue);
initial2.returnOptionalStruct(true, out ofs);
test(!ofs.HasValue);

Test.G g = new Test.G();
g.gg1Opt = new Ice.Optional<Test.G1>(new Test.G1("gg1Opt"));
Expand Down Expand Up @@ -451,16 +451,16 @@ public class AllTests : global::Test.AllTests
}
output.WriteLine("ok");

output.Write("testing marshaling of objects with optional objects...");
output.Write("testing marshaling of objects with optional members...");
output.Flush();
{
Test.F f = new Test.F();

f.af = new Test.A();
f.ae = (Test.A)f.af;
f.fsf = new Test.FixedStruct();
f.fse = (Test.FixedStruct)f.fsf;

Test.F rf = (Test.F)initial.pingPong(f);
test(rf.ae == rf.af.Value);
test(rf.fse == rf.fsf.Value);

factory.setEnabled(true);
os = new Ice.OutputStream(communicator);
Expand All @@ -475,7 +475,7 @@ public class AllTests : global::Test.AllTests
@in.endEncapsulation();
factory.setEnabled(false);
rf = ((FValueReader)rocb.obj).getF();
test(rf.ae != null && !rf.af.HasValue);
test(rf.fse != null && !rf.fsf.HasValue);
}
output.WriteLine("ok");

Expand Down Expand Up @@ -1867,27 +1867,26 @@ public class AllTests : global::Test.AllTests
@in.endEncapsulation();

Test.F f = new Test.F();
f.af = new Test.A();
f.af.Value.requiredA = 56;
f.ae = f.af.Value;
f.fsf = new Test.FixedStruct(56);
f.fse = f.fsf.Value;

os = new OutputStream(communicator);
os.startEncapsulation();
os.writeOptional(1, OptionalFormat.Class);
os.writeValue(f);
os.writeOptional(2, OptionalFormat.Class);
os.writeValue(f.ae);
os.writeOptional(2, OptionalFormat.VSize);
os.writeSize(4);
Test.FixedStruct.ice_write(os, f.fse);
os.endEncapsulation();
inEncaps = os.finished();

@in = new InputStream(communicator, inEncaps);
@in.startEncapsulation();
test(@in.readOptional(2, OptionalFormat.Class));
ReadValueCallbackI rocb = new ReadValueCallbackI();
@in.readValue(rocb.invoke);
test(@in.readOptional(2, OptionalFormat.VSize));
@in.skipSize();
Test.FixedStruct fs1 = Test.FixedStruct.ice_read(@in);
@in.endEncapsulation();
Test.A a = (Test.A)rocb.obj;
test(a != null && a.requiredA == 56);
test(fs1 != null && fs1.m == 56);
}

{
Expand Down Expand Up @@ -2333,15 +2332,13 @@ public override void read(Ice.InputStream @in)
_f = new Test.F();
@in.startValue();
@in.startSlice();
// Don't read af on purpose
//in.read(1, _f.af);
// Don't read fsf on purpose
//@in.read(1, _f.fsf);
@in.endSlice();
@in.startSlice();
ReadValueCallbackI rocb = new ReadValueCallbackI();
@in.readValue(rocb.invoke);
_f.fse = Test.FixedStruct.ice_read(@in);
@in.endSlice();
@in.endValue();
_f.ae = (Test.A)rocb.obj;
}

public Test.F getF()
Expand Down
Loading
Loading