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

test(rosetta): Extend submodule import test case for nested structs in submodules #4056

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ansgarm
Copy link
Contributor

@ansgarm ansgarm commented Apr 13, 2023

Hi! I created this (failing) test case for a jsii-rosetta problem we are seeing in cdktf convert (bullet point # 2 in this comment).

Originally we uncovered that this code was invalid for Python but upon writing this test case, I think it is also producing invalid code for C# and Go. I'm not entirely sure whether I've adjusted the expected results in the right way, though. I'll add comments to those places including what Rosetta currently returns to make it easier to see how these tests currently fail.

I'd be open to contribute to a solution but I'm having a hard time even describing the problem in the right terms 😅


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -9,7 +9,7 @@
});

// Accesses two distinct points of the submodule hierarchy
var myClass = new Submodule.MyClass(new SomeStruct { Prop = Submodule.Child.SomeEnum.SOME });
var myClass = new Submodule.MyClass(new SomeStruct { Prop = Submodule.Child.SomeEnum.SOME, NestedStruct = new Submodule.Child.AnotherStruct { StringProperty = "hello" } });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This currently fails as the result from Rosetta is missing the Submodule.Child. for AnotherStruct
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While looking at this once more, I noticed that the original test already seems to have a small error, too:

Suggested change
var myClass = new Submodule.MyClass(new SomeStruct { Prop = Submodule.Child.SomeEnum.SOME, NestedStruct = new Submodule.Child.AnotherStruct { StringProperty = "hello" } });
var myClass = new Submodule.MyClass(new Submodule.Child.SomeStruct { Prop = Submodule.Child.SomeEnum.SOME, NestedStruct = new Submodule.Child.AnotherStruct { StringProperty = "hello" } });

Comment on lines +16 to +18
NestedStruct: &AnotherStruct{
stringProperty: jsii.String("hello"),
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This currently fails as the result from Rosetta is returning anotherStruct instead of AnotherStruct
image

@@ -9,7 +9,7 @@
)

# Accesses two distinct points of the submodule hierarchy
my_class = calc.submodule.MyClass(prop=calc.submodule.child.SomeEnum.SOME)
my_class = calc.submodule.MyClass(prop=calc.submodule.child.SomeEnum.SOME, nested_struct=calc.submodule.child.AnotherStruct(string_property="hello"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This currently fails as the result from Rosetta is missing the calc.submodule.child. for AnotherStruct
image

@ansgarm
Copy link
Contributor Author

ansgarm commented Apr 13, 2023

@DanielMSchmidt just pointed me at https://github.com/aws/jsii-compiler which is where JSII 5.0 lives – I'll create a new PR over there as we're on JSII 5.0 now 😄

Edit: I'll create it in https://github.com/aws/jsii-rosetta of course 😄

@ansgarm
Copy link
Contributor Author

ansgarm commented Apr 13, 2023

Hm, I could use some help there – it seems like jsii-calc still needs to be updated in this repo, right?
In that case, I won't create that other PR just yet, as it won't really work (= fail successfully) anyway yet 😅

@RomainMuller
Copy link
Contributor

@ansgarm the jsii compiler 5.0.x lives in aws/jsii-compiler, and the jsii-rosetta 5.0.x lives in aws/jsii-rosetta.

The jsii-calc package (and dependencies) are "copied" in aws/jsii-compiler (under the fixtures directory), and have been slightly edited there to improve the v5.0 coverage... Unfortunately, since they need to assess some backwards-compatibility behavior, the code there had to be updated to include TypeScript 3.9-incompatible syntax... So it's not just a matter of copying the code over.

If jsii-calc here is lacking some APIs you need for your tests, you could just add the necessary changes into this PR...

@ansgarm ansgarm force-pushed the rosetta-python-nested-structs-bug branch from fc24685 to 2919522 Compare July 10, 2023 09:04
@mrgrain mrgrain marked this pull request as draft June 26, 2024 12:10
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.

2 participants