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

[#213] test: add test cases #439

Merged

Conversation

xieyuschen
Copy link
Contributor

@xieyuschen xieyuschen commented Oct 6, 2024

Notes for Reviewer

I have added some test cases during I'm learning the codes. It improves out test.

Pre-Review Checklist for the PR Author

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked in the References section
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Updates #213

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.02%. Comparing base (a7f0df7) to head (c472781).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #439      +/-   ##
==========================================
+ Coverage   78.93%   79.02%   +0.08%     
==========================================
  Files         196      196              
  Lines       23283    23366      +83     
==========================================
+ Hits        18379    18465      +86     
+ Misses       4904     4901       -3     
Files with missing lines Coverage Δ
iceoryx2/src/service/builder/publish_subscribe.rs 90.97% <ø> (-0.57%) ⬇️
.../src/service/static_config/message_type_details.rs 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@xieyuschen
Copy link
Contributor Author

oops, I have found that alignment in 32bits is 4, not 8. let me change it:)

Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

@xieyuschen It is always a pleasure to start the week with an Earl Grey Hot (with milk) and a review of your PR.

Thank you very much for your continuous effort and helping us out!

iceoryx2/src/service/static_config/message_type_details.rs Outdated Show resolved Hide resolved
iceoryx2/src/service/static_config/message_type_details.rs Outdated Show resolved Hide resolved
iceoryx2/src/service/static_config/message_type_details.rs Outdated Show resolved Hide resolved
};
assert_that!(want, eq got);

let got = MessageTypeDetails::from::<i32, bool, String>(TypeVariant::Dynamic);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using String in iceoryx2 would lead to a segmentation fault. But you could use [MyPayload] as dynamic slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understood, btw not sure you are refering to the usage of [MyPayload] like this:

MessageTypeDetails::from::<i32, bool, &[MyPayload]>(TypeVariant::Dynamic);

Using String in iceoryx2 would lead to a segmentation fault

I think we can try our best to reject the some types such as string in MessageTypeDetails::from, such as panic.
Not sure we can have some refinements here, but i will have a look later. How do you think about it?

Copy link
Contributor

@elfenpiff elfenpiff Oct 7, 2024

Choose a reason for hiding this comment

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

@xieyuschen The usage would be like:

MessageTypeDetails::from::<i32, bool, [MyPayload]>(TypeVariant::Dynamic);

I think we can try our best to reject the some types such as string in MessageTypeDetails::from, such as panic.
Not sure we can have some refinements here, but i will have a look later. How do you think about it?

The idea was to provide a trait ShmSend and a corresponding proc macro. Every type that implements ShmSend can be used as payload type. We provide ShmSend for all POD types (u64, u32, bool ...) and all shared memory compatible containers from iceoryx2_bb_containers. The proc macro would also add #[repr(C)].

So the user would annotate the struct with #[ShmSend] and if one member does not implement it, it would fail at compile time - for instance if one member would be a String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, that's great, could I know the issue to track this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @elfenpiff looks your example here doesn't work. but never mind, I can skip it in this PR and test it in the following PR.

        #[repr(C)]
        struct MyPayload {
            header: i32,
            _user_header: i32,
            payload: i32,
        }
        let details = MessageTypeDetails::from::<i32, i32, [MyPayload]>(TypeVariant::Dynamic);
error[E0277]: the size for values of type `[service::static_config::message_type_details::tests::test_payload_ptr_from_header::MyPayload]` cannot be known at compilation time
   --> iceoryx2/src/service/static_config/message_type_details.rs:283:60
    |
283 |         let details = MessageTypeDetails::from::<i32, i32, [MyPayload]>(TypeVariant::Dynamic);
    |                                                            ^^^^^^^^^^^ doesn't have a size known at compile-time

iceoryx2/src/service/static_config/message_type_details.rs Outdated Show resolved Hide resolved
iceoryx2/tests/service_static_config_tests.rs Outdated Show resolved Hide resolved
@xieyuschen
Copy link
Contributor Author

@xieyuschen It is always a pleasure to start the week with an Earl Grey Hot (with milk) and a review of your PR.

Thank you very much for your continuous effort and helping us out!

ack, will try to resolve it later today and thanks for your clarify.

@xieyuschen xieyuschen force-pushed the iox2-213-add-more-test-cases branch 2 times, most recently from 720a7ce to 85c45ce Compare October 7, 2024 14:49
@xieyuschen
Copy link
Contributor Author

xieyuschen commented Oct 7, 2024

Hi @elfenpiff , I have updated the PR with multiple small commits for you to check them based on your comments. I resolved the straitfoward comments and left some comments for you to see whether you have concerns about it.

Thanks for reviewing.

@xieyuschen xieyuschen force-pushed the iox2-213-add-more-test-cases branch 2 times, most recently from e431e64 to 1cb02d7 Compare October 7, 2024 15:33
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 7, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 7, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 7, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 7, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 8, 2024
Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

I left a comment on how to fix the ubuntu 32 bit issue. When this is fixed we can merge this PR.

iceoryx2/tests/service_static_config_tests.rs Outdated Show resolved Hide resolved
@xieyuschen
Copy link
Contributor Author

Hi @elfenpiff , I have squashed the commits together for your final review. Could you review it and then we can merge it?

I plan to create another new PRs for the testing later. I found it helps me to understand the project while improving our codebase as well.

elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 10, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 10, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
elBoberido added a commit to elBoberido/iceoryx2 that referenced this pull request Oct 11, 2024
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