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

[20424] Dynamic language binding tests #4626

Merged
merged 52 commits into from
May 14, 2024

Conversation

JLBuenoLopez
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez commented Mar 26, 2024

Description

This PR adds types that test the Dynamic Language Binding.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • N/A Any new/modified methods have been properly documented using Doxygen.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • N/A New feature has been added to the versions.md file (if applicable).
  • N/A New feature has been documented/Current behavior is correctly described in the documentation.
  • N/A Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@JLBuenoLopez JLBuenoLopez added this to the v3.0.0 milestone Mar 26, 2024
@JLBuenoLopez
Copy link
Contributor Author

JLBuenoLopez commented Mar 26, 2024

The following suggestions should be taken into account:

Base automatically changed from feature/dynamic-type-builder-factory-create-type-w-type-object-impl to feature/xtypes1.3 March 27, 2024 15:33
Copy link
Contributor Author

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

Partial review

Comment on lines 2504 to 7773
{
eprosima::fastdds::dds::Log::ScopeLogs _("disable");
// Try to add a descriptor with the same name.
member_descriptor = traits<MemberDescriptor>::make_shared();
member_descriptor->type(factory->get_primitive_type(eprosima::fastdds::dds::TK_INT32));
member_descriptor->name("THIRD");
EXPECT_NE(builder->add_member(member_descriptor), RETCODE_OK);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should add some more enum MemberDescriptor consistency checks apart from the non-empty name suggested here:

  • Use of default_value_ to give the enumerator a specific value different from the sequential one automatically assigned. Add another literal and check that the sequential order restarts with the previous literal value.
  • MemberId taking a value different from MEMBER_ID_INVALID: MemberDescriptor should be inconsistent.
  • is_default_label_ flag should be false according to the specification, but I think this field should be used to apply the @default_literal builtin annotation.
  • is_key_ flag should be false as this annotation only applies to Structure members and Union discriminators.
  • is_must_understand_ flag should be false as this builtin annotation only applies to Structure members.
  • is_optional_ flag should be false as this builtin annotation only applies to Structure members.
  • is_shared_ flag should be false as this annotation only applies to Structure members.
  • label property should be empty in order for the literal to be consistent.
  • try_construct_kind does not apply, but the enumerator which is defined in the IDL does not allow for this case. It is already initialized to DISCARD so I am not sure if a consistency check can be done with this property.

Copy link
Member

Choose a reason for hiding this comment

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

Done except default_value_ beucase that logic is not implemented yet.

test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

Another DynamicTypesTests partial review

test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
@JLBuenoLopez JLBuenoLopez force-pushed the feature/tests_for_dds_types_test branch from c53092e to 3f62216 Compare April 3, 2024 07:37
@adriancampo adriancampo force-pushed the feature/tests_for_dds_types_test branch from 3f62216 to fd95613 Compare April 3, 2024 10:05
Base automatically changed from feature/xtypes1.3 to 3.0.x-devel April 5, 2024 05:39
@JLBuenoLopez
Copy link
Contributor Author

This PR should fix inheritance (both structure and bitsets) when loading a Dynamic Type from a XML profiles file.

Copy link
Contributor Author

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

Partial review

test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
Comment on lines 5604 to 8323
ASSERT_TRUE(struct_data);

// Test get_member_by_name and get_member_by_index.
ASSERT_EQ(MEMBER_ID_INVALID, struct_data->get_member_id_by_name(""));
ASSERT_EQ(0, struct_data->get_member_id_by_name("Structure"));
ASSERT_EQ(10, struct_data->get_member_id_by_name("int64"));
ASSERT_EQ(0, struct_data->get_member_id_at_index(0));
ASSERT_EQ(10, struct_data->get_member_id_at_index(1));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is not correct. Getting the complex value for a non-selected union member fails. I would expect the same to happen for loans. And if we decide that the loan can be returned and if set, the discriminator points to the new union member, then the same philosophy must be followed with complex API.

Also, the test only uses the get API and this should fail because the union member being read is not the one selected.

test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

Final review for DynamicTypesTests

test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
test/feature/dynamic_types/DynamicTypesTests.cpp Outdated Show resolved Hide resolved
@JLBuenoLopez
Copy link
Contributor Author

XML structure and bitset inheritance must be fixed. Please, open also a documentation PR fixing the corresponding snippets.

Copy link
Contributor Author

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

Partial review. Only maps.idl tests pending for review.

Copy link
Contributor Author

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

Partial review

Copy link
Contributor Author

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

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

Final review for my part.

static_pubsubType);
EXPECT_EQ(value.size(), struct_data.var_map_long_double().size());
for (auto const& map_element : value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been rereading the specification and what it says is:

Map keys are implicitly converted to strings

But the specification does not specify which kind of string. So I suppose, that we should either define both APIs with std::string or just std::wstring. This change would introduce support to keys with wide string keys and wide string annotation parameters

@EduPonz EduPonz force-pushed the 3.0.x-devel branch 2 times, most recently from 8c0830d to 5148f5d Compare April 18, 2024 13:08
adriancampo and others added 15 commits May 13, 2024 11:35
Signed-off-by: adriancampo <adriancampo@eprosima.com>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González <ricardo@richiware.dev>
Signed-off-by: Ricardo González <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
@richiware richiware force-pushed the feature/tests_for_dds_types_test branch from 5220896 to 7801390 Compare May 13, 2024 09:42
@richiware richiware requested review from richiware and removed request for richiware May 13, 2024 09:42
Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
@richiware richiware force-pushed the feature/tests_for_dds_types_test branch from 7801390 to be61920 Compare May 13, 2024 10:22
@richiware richiware requested review from richiware and removed request for richiware May 13, 2024 10:22
@richiware richiware changed the base branch from 3.0.x-devel to master May 14, 2024 06:33
@richiware richiware dismissed their stale review May 14, 2024 06:33

The base branch was changed.

@MiguelCompany MiguelCompany changed the base branch from master to 3.0.x-devel May 14, 2024 07:01
@MiguelCompany MiguelCompany changed the base branch from 3.0.x-devel to master May 14, 2024 07:01
@richiware richiware merged commit bc03220 into master May 14, 2024
7 of 12 checks passed
@richiware richiware deleted the feature/tests_for_dds_types_test branch May 14, 2024 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-pending PR which CI is running
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants