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

Change CI to dockers #239

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Change CI to dockers #239

merged 1 commit into from
Sep 8, 2022

Conversation

fmrico
Copy link
Contributor

@fmrico fmrico commented Sep 8, 2022

Hi

This PR is for fixing CI, which has been broken since some weeks: ros-tooling/action-ros-ci#768

Best

Signed-off-by: Francisco Martín Rico [email protected]

Signed-off-by: Francisco Martín Rico <[email protected]>
@fmrico fmrico merged commit d7f606e into master Sep 8, 2022
@fmrico fmrico deleted the fix_ci1 branch September 8, 2022 19:14
@sarcasticnature
Copy link
Contributor

hmmmm, it looks like the on_feedback test in plansys2_bt_actions is causing a failure (timing out) in the master CI... not quite sure why. The amount of time it took the test to succeed is suspect even in this PR's (successful) test. I don't think I've ever encountered the issue on my machine and am wondering if the test only has problems when operating on a slower runner.

Will investigate either tonight or this weekend.

@fmrico
Copy link
Contributor Author

fmrico commented Sep 9, 2022

Thanks, @sarcasticnature !!

This issue blocks the PRs, and it's been a pain in the ass to fix it. I merged this PR because it passed the tests, but it failed again in the main...

@sarcasticnature
Copy link
Contributor

Alright, think I found the problem. The plansys2_bt_actions unit tests create and interact w/ a BT node outside of a tree and we were calling tick() instead of executeTick() to manually 'run' the node. The problem with the former is that it does not update the node's internal status (which the latter does), meaning that goals were being repeatedly sent to the action server because the node wasn't aware that it had already sent one.

If your system is fast (or you're just lucky) the test will likely eventually go through, but it's technically a race condition as is. I have pushed an update to my PR (#236) which should resolve this issue

@fmrico
Copy link
Contributor Author

fmrico commented Sep 12, 2022

Thanks, @sarcasticnature!!! good catch ;)

As you push that commit in a merged PR, could you please make a PR only with the fix?

Thanks again

@sarcasticnature
Copy link
Contributor

I will try and get around to that tonight

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