[#476] Introduce test feature in iceoryx-bb for bump_allocator

[xieyuschen]

Notes for Reviewer

See proposal in the issue. By the way, could I know the rules/requirements to be met for a collaborator? Currently, I cannot assign you guys as reviewers:(

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
   • Commit messages have the issue ID ([#123] Add posix ipc example)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
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

Closes #476

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.22%. Comparing base (5f6a0dc) to head (8ffa846).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #478      +/-   ##
==========================================
+ Coverage   79.19%   79.22%   +0.02%     
==========================================
  Files         200      200              
  Lines       23716    23716              
==========================================
+ Hits        18781    18788       +7     
+ Misses       4935     4928       -7     

Files with missing lines	Coverage Δ
iceoryx2-bb/elementary/src/bump_allocator.rs	86.36% <ø> (ø)

... and 3 files with indirect coverage changes

[elfenpiff]

@xieyuschen I really like the approach - great idea! I am pinging @elBoberido since he is a bit more familiar with the idioms of Rust.

[elBoberido]

This will make the features available to all user of iceoryx2-bb-elementary, at least in this workspace. I'm not sure what happens with external users. See also https://doc.rust-lang.org/cargo/reference/features.html#feature-unification

Could you check if an external crate that depends on container and elementary has access to the BumpAllocator without specifying the feature flag itself?

A workaround could be to add iceoryx2-bb-elementary = { workspace = true , features = ["iox2-test"]} as dev dependency and leave the regular dependency without the flag. One could even go one step further and just add iceoryx2-bb-elementary with the flag as regular dependency to iox2-bb-testing and re-export it in that module. Since there would be no transitive dependency on a iceoryx-bb-elementary with the flag set, except the dev targets, regular users of the crate should not have access to the BumpAllocator by accident.

What do you think?

I still need to test this with bazel tomorrow.

[xieyuschen]

My bad, the workspace usage actually enables this feature to all users. But we can enable the feature inside the dev-dependency so it could solve this issue here perfectly.

If an external user runs cargo build, rustc fails and reports the errors. This satisfy our requirement:

• internal testing cases doesn't be affected
• outside users cannot use it except explicit enable the testing feature

error[E0433]: failed to resolve: could not find `bump_allocator` in `iceoryx2_bb_elementary`
  --> src/main.rs:6:36
   |
6  |     let _a=iceoryx2_bb_elementary::bump_allocator::BumpAllocator::new(8);
   |                                    ^^^^^^^^^^^^^^ could not find `bump_allocator` in `iceoryx2_bb_elementary`
   |
note: found an item that was configured out
  --> /Users/yuchen.xie/.cargo/git/checkouts/iceoryx2-d2f97997d049ef12/e3225b5/iceoryx2-bb/elementary/src/lib.rs:23:9
   |
23 | pub mod bump_allocator;
   |         ^^^^^^^^^^^^^^
note: the item is gated behind the `testing` feature
  --> /Users/yuchen.xie/.cargo/git/checkouts/iceoryx2-d2f97997d049ef12/e3225b5/iceoryx2-bb/elementary/src/lib.rs:22:7
   |
22 | #[cfg(feature = "testing")]

external user cargo toml

[package]
name = "iceoryx2-playground"
version = "0.1.0"
edition = "2021"

[dependencies]
iceoryx2 = { git = "https://github.com/xieyuschen/iceoryx2", rev = "e3225b55" }
iceoryx2-bb-elementary = { git = "https://github.com/xieyuschen/iceoryx2", rev = "e3225b55" }

main.rs

use core::time::Duration;
use iceoryx2::prelude::*;
const CYCLE_TIME: Duration = Duration::from_secs(1);

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let _a=iceoryx2_bb_elementary::bump_allocator::BumpAllocator::new(8);
    let node = NodeBuilder::new().create::<ipc::Service>()?;

    let service = node.service_builder(&"My/Funk/ServiceName".try_into()?)
        .publish_subscribe::<usize>()
        .open_or_create()?;

    let publisher = service.publisher_builder().create()?;

    while node.wait(CYCLE_TIME).is_ok() {
        let sample = publisher.loan_uninit()?;
        let sample = sample.write_payload(1234);
        sample.send()?;
    }

    Ok(())
}

[elBoberido]

@xieyuschen it's caused by an update of the nightly toolchain. A few months ago we alreasy added a workaround to those two tests. Let's wait for a few days to see if another nightly toolchain update fixes the problem. If it persists, we will have a closer look again.

Btw, I still did not manage to check whether this approach works with bazel. I'm also on the ROSCon the next days, so it might take till Friday until I have time

[elBoberido]

You basically need to run bazel test //... on your branch after rebasing to main, once #482 is merged.
With the feature, it will fail and you need to add crate_features = [ "testing" ], to the rust_test_suite of the failing tests, right below srcs to kerp it consistent with other cases.

[elBoberido]

@xieyuschen it's now merged and you can try your luck. After writing the last comment, I'm convinced it should work without issues.

[xieyuschen]

Hi @elBoberido , i have change the bazel code and verify it could pass the test in my ubuntu so i think it's ready for you to review.
The bazel doesn't work in my MacOS, but we can discuss it more in the issue so we can focus on the topic of this PR. thanks!

[elBoberido]

@xieyuschen it works great on bazel, even in external builds. On main there are some more cases that use the

#[doc(hidden)]
pub mod testing;

pattern. Would be great if you could also adjust those cases and also add an entry to the FAQ_ICEORYX_DEVS.md

[xieyuschen]

@xieyuschen it works great on bazel, even in external builds. On main there are some more cases that use the

#[doc(hidden)]
pub mod testing;

pattern. Would be great if you could also adjust those cases and also add an entry to the FAQ_ICEORYX_DEVS.md

Probably we can merge this first so you needn't take efforts to see this part of change again when i do the code change for the other cases. HDYT @elBoberido

Anyway, I will do the left things tomorrow.

[elBoberido]

Okay, I think here we have a problem. I totally overlooked that this one needs to be part of rust_test_suite. But in that won't work and if it is part of rust_library all consumers of iceoryx2-bb-elementary will have access to the BumpAllocator.

It seems what we want to achieve is not compatible with bazel :/

[xieyuschen]

probably we can define 2 rust_library for different purposes, one for users and one for testing. I will try it today to see whether it works or not.

[xieyuschen]

@elBoberido never mind, I have defined a new rust_library inside elementary with testing feature and define an alias inside container for the test cases. So it doesn't affect the common rust_library definitions.

[xieyuschen]

hi @elBoberido I have enabled the testing feature for test_suite only, so I believe there is no concern now:)

Please let me know whether you like this current approach, or we have a better way to do so. Then, after merging this PR and #487, I can support the others in this approach as well.

[xieyuschen]

hi @elBoberido , could you kindly review it when you have time? Thanks:)

[elBoberido]

@xieyuschen for now, I would just keep everything as is instead of adding a second rust_library. It's not only this one case but we use the doc hidden flag also in other places and this would result in quite a few lib duplications.

[xieyuschen]

hi @elBoberido , I think this is limited by bazel itself, we cannot enable a feature at the consumer side, ref.

In Bazel you can not select features of library in the consumer of the library, features should be defined in the library itself.

I don't like defining one more rust_library for testing as well, but that's the best way in bazel now. But given the case that the original goal is to hide test structures by adding new feature, looks like we don't have a consensus that whether this change makes things better.

Thanks for your review.

[elBoberido]

@xieyuschen I would suggest to keep this PR open and review the decision for the v0.6 development cycle.