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

Trying to figure out which tests are flaky and fix, adding debug info… #140

Merged
merged 29 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
b411bea
Trying to figure out which tests are flaky and fix, adding debug info…
Oct 5, 2023
8c32d47
Add rust version confirmation to CI
Oct 5, 2023
869085d
Nuking rosapi, and the funky macro
Oct 8, 2023
d8540c9
Fix ros1_test
Oct 8, 2023
26c92d5
Merge in master
Oct 8, 2023
6bdceae
Lint
Oct 8, 2023
269855a
Trying to get more info on failure
Oct 8, 2023
bfea3c1
Disabing previously ignored test so I can test what I really want to
Oct 9, 2023
ff6a7d3
Ignore is messy as it turns out
Oct 9, 2023
4c858f0
Trying to get some debug out of these tests
Oct 9, 2023
f876f67
Convert to multi-thread test
Oct 9, 2023
21dd3b0
WAS IT DNS THE WHOLE TIME
Oct 9, 2023
4410665
Removing multi-threaded test
Oct 9, 2023
131ab1d
Port fix
Oct 9, 2023
e4623d6
try_bind instead of bind for xmlrpc server for better error propagation
Oct 9, 2023
cb10c20
Disable spurious test case
Oct 9, 2023
1f73209
Adding back in multi-thread executor
Oct 9, 2023
eb0e4c7
Add delay to ensure shutdown completes
Oct 9, 2023
8b43d2e
FOUND IT BITCHES :poop:
Oct 11, 2023
55a7df7
Commiting in-flight work to transfer between computers
Oct 22, 2023
40a7015
Adding yet more debug
Oct 22, 2023
48fdc3a
Seemed to actually have fixed the error (at least for one test) by sw…
Oct 22, 2023
99d9e78
Remove unneeded dashmaps
Oct 22, 2023
75354b2
Undoing a lot of the sins committed while debugging
Oct 22, 2023
de44534
Re-enabling rosapi
Oct 22, 2023
b27a872
Rosapi fix
Oct 22, 2023
70d7a67
Adding rosapi back in for ros1 integration tests, experimenting with …
Oct 22, 2023
e060f30
Turning single threaded execution back on
Oct 22, 2023
f1090e2
Cleanups from MR review
Oct 23, 2023
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
2 changes: 1 addition & 1 deletion .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
`Insert description`

## Fixes
Issue Number: `number`
Closes: `number`

## Checklist
- [ ] Update CHANGELOG.md
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/galactic.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ on:

env:
HOME: /root
# Coupled with our use of the test_log crate this should give us good CI output on failure
RUST_LOG: debug

jobs:
galactic:
Expand All @@ -22,6 +24,8 @@ jobs:
uses: actions/checkout@v3
with:
submodules: 'true'
- name: Verify rust version
run: source /root/.cargo/env; rustc --version
- name: Lint
run: source /root/.cargo/env; cargo fmt --all -- --check
- name: Build Main Lib
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/humble.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ on:

env:
HOME: /root
# Coupled with our use of the test_log crate this should give us good CI output on failure
RUST_LOG: debug

jobs:
humble:
Expand All @@ -22,6 +24,8 @@ jobs:
uses: actions/checkout@v3
with:
submodules: 'true'
- name: Verify rust version
run: source /root/.cargo/env; rustc --version
- name: Lint
run: source /root/.cargo/env; cargo fmt --all -- --check
- name: Build Main Lib
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/noetic.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ on:
env:
HOME: /root
ROS_PACKAGE_PATH: /opt/ros/noetic/share
# Coupled with our use of the test_log crate this should give us good CI output on failure
RUST_LOG: debug

jobs:
noetic:
Expand All @@ -23,6 +25,8 @@ jobs:
uses: actions/checkout@v3
with:
submodules: 'true'
- name: Verify rust version
run: source /root/.cargo/env; rustc --version
- name: Lint
run: source /root/.cargo/env; cargo fmt --all -- --check
- name: Build Main Lib
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/noetic_gencpp.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ on:

env:
HOME: /root
# Coupled with our use of the test_log crate this should give us good CI output on failure
RUST_LOG: debug

jobs:
noetic_gencpp:
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- Removed `find_and_generate_ros_messages_relative_to_manifest_dir!` this proc_macro was changing the current working directory of the compilation job resulting in a variety of strange compilation behaviors. Build.rs scripts are recommended for use cases requiring fine
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it was necessary to remove this macro entirely. It just didn't need to be changing the working directory. Like it just need to prepend the provided paths in the macro with the manifest directory path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to remove because I do think it is just a bad API (like the method name is a joke) we could have fixed it, but I really think this macro came about from me not understanding aspects of when macros were expanded and what working directory / cargo manifest dir were under different circumstances.

I think ultimately we figure out a new API in the future for the same reason.

grained control of message generation.
- The function interface for top level generation functions in `roslibrust_codegen` have been changed to include the list of dependent
filesystem paths that should trigger re-running code generation. Note: new files added to the search paths will not be automatically detected.

Expand Down
4 changes: 2 additions & 2 deletions roslibrust/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ simple_logger = "2.1.0"

[features]
default = []
# Provides a rosapi rust interface
rosapi = []
# Note: all does not include running_bridge as that is only intended for CI
all = []
# Provides a rosapi rust interface
rosapi = []
# Intended for use with tests, includes tests that rely on a locally running rosbridge
running_bridge = []
# For use with integration tests, indicating we are testing integration with a ros1 bridge
Expand Down
2 changes: 1 addition & 1 deletion roslibrust/examples/native_ros1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

#[cfg(feature = "ros1")]
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make the return of main have these bounds? I think it's just noise.

Copy link
Collaborator 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 punt to #118

I ended up having to add this bound in a number of places to make it valid to return the errors from tasks.

use roslibrust::NodeHandle;

simple_logger::SimpleLogger::new()
Expand Down
2 changes: 1 addition & 1 deletion roslibrust/examples/ros1_listener.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ roslibrust_codegen_macro::find_and_generate_ros_messages!("assets/ros1_common_in

#[cfg(feature = "ros1")]
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
use roslibrust::NodeHandle;

simple_logger::SimpleLogger::new()
Expand Down
2 changes: 1 addition & 1 deletion roslibrust/examples/ros1_talker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ roslibrust_codegen_macro::find_and_generate_ros_messages!("assets/ros1_common_in

#[cfg(feature = "ros1")]
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
use roslibrust::NodeHandle;

simple_logger::SimpleLogger::new()
Expand Down
1 change: 1 addition & 0 deletions roslibrust/src/ros1/master_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ impl MasterClient {
self.client_uri.clone().into(),
],
)?;
log::trace!("Posting {body:?} to register publisher");
self.post(body).await
}

Expand Down
Loading