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.

Sign up for GitHub

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

Jump to bottom

Unit tests for all nodes #61

Merged
merged 12 commits into from Jun 22, 2023
Merged

Unit tests for all nodes #61

merged 12 commits into from Jun 22, 2023

Conversation

mllofriu
Copy link
Contributor

@mllofriu mllofriu commented May 5, 2023

Following the POC of #59, I implemented unit tests for all nodes.

As explained there, I'm trying to implement the feature request I created here - #58.

In doing so, I found myself refactoring code in tool_base_node.

In order to prevent breaking things, I figured I'd write some unit tests for the existing functionality.

I'm creating this pull request as a draft to get some early feedback. If we agree on it, I can apply the same pattern to the other tools in topic_tools.

@mllofriu mllofriu requested a review from a team as a code owner May 5, 2023 19:25
@MichaelOrlov MichaelOrlov requested review from emersonknapp and christophebedard and removed request for gbiggs May 18, 2023 20:05
dependabot bot and others added 8 commits May 23, 2023 11:55
@dependabot @mllofriu
Bump hmarr/auto-approve-action from 3.2.0 to 3.2.1
ac60ecb 
Bumps [hmarr/auto-approve-action](https://github.com/hmarr/auto-approve-action) from 3.2.0 to 3.2.1.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/hmarr/auto-approve-action/releases">hmarr/auto-approve-action's releases</a>.</em></p>
<blockquote>
<h2>v3.2.1</h2>
<h2>What's Changed</h2>
<ul>
<li>Only consider the latest review for a user when deciding whether to re-review by <a href="https://github.com/hmarr"><code>@​hmarr</code></a> in <a href="https://redirect.github.com/hmarr/auto-approve-action/pull/216">hmarr/auto-approve-action#216</a></li>
</ul>
<p><strong>Full Changelog</strong>: <a href="https://github.com/hmarr/auto-approve-action/compare/v3.2.0...v3.2.1">https://github.com/hmarr/auto-approve-action/compare/v3.2.0...v3.2.1</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/hmarr/auto-approve-action/commit/44888193675f29a83e04faf4002fa8c0b537b1e4"><code>4488819</code></a> Only consider the latest review for a user (<a href="https://redirect.github.com/hmarr/auto-approve-action/issues/216">#216</a>)</li>
<li>See full diff in <a href="https://github.com/hmarr/auto-approve-action/compare/v3.2.0...v3.2.1">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=hmarr/auto-approve-action&package-manager=github_actions&previous-version=3.2.0&new-version=3.2.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>

Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Add a basic unit test for relay_node
3b2d392 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Add a basic unit test for relay_node
885f77d 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Generalize the test fixture to use in other tests
db77173 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Add a test for drop
3b859a5 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Add a test for the throttle node
e384c45 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Refactor TestTopicTool to allow for the creation of a multi sub tool
b20e90d 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Add a test for mux. Add a call to re-suscribe when the topic is switched
55702dd 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Copy link
Contributor Author

@christophebedard, this is the new version of #59

@mllofriu
Copy link
Contributor Author

@MichaelOrlov, is there anything I can do to facilitate reviews to this PR?

@christophebedard
Copy link
Member

Sorry for the delay, I will review this PR on/by Thursday.

@christophebedard
Copy link
Member

@emersonknapp are we aiming to support multiple distros from this repo's main branch? Since that's currently the only branch in this repo.

@christophebedard
Copy link
Member

@emersonknapp are we aiming to support multiple distros from this repo's main branch? Since that's currently the only branch in this repo.

rosdistro shows that the same version is released in a few different distros, so I guess that's a yes.

@christophebedard
Copy link
Member

Other than some minor comments, this looks good to me!

@emersonknapp
Copy link
Contributor

@christophebedard that wasn't really the intent. It looks like it's now released into Humble, Iron, and Rolling on the main branch but I'm not really positive those will all successfully build (though I think we aren't using any fancy new features yet). We should probably cut release branches for those distros and start keeping a backport regimen. It's just a pretty new package so it didn't come up yet. If we wanted to, we could try a multi-distro branch with ifdef feature flags, but it might be easier to follow the standard route.

@mllofriu sorry for the slow review - I was on vacation for most of May and just didn't get around to it. I'll take a look today.

@emersonknapp emersonknapp removed the request for review from Karsten1987 June 6, 2023 16:03
emersonknapp
emersonknapp approved these changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

I'll call this "approved once feedback addressed" - it's mostly small stuff, overall this looks good and thank you for doing it

topic_tools/CMakeLists.txt Outdated Show resolved Hide resolved
topic_tools/test/test_drop.cpp Outdated Show resolved Hide resolved
topic_tools/test/test_drop.cpp Outdated Show resolved Hide resolved
topic_tools/test/test_mux.cpp Outdated Show resolved Hide resolved
topic_tools/test/test_throttle.cpp Outdated Show resolved Hide resolved
topic_tools/test/test_throttle.cpp Outdated Show resolved Hide resolved
@mllofriu
Use native cmake over ament
2b07424 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Add variables to avoid magic numbers in tests
0ea3d8e 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Add spaces in header files
0aebd9a 
Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Remove conditional depends.
910de14 
Fix some commennts.

Signed-off-by: Martin Llofriu <mllofriu@gmail.com>
@mllofriu
Copy link
Contributor Author

@emersonknapp @christophebedard, I think this is ready. Thanks for the thorough review!

@mllofriu
Copy link
Contributor Author

@emersonknapp @christophebedard, do you understand why the build is failing right now? It's building for me on a humble dev docker.

@christophebedard
Copy link
Member

I am not sure. Builds fine for me locally on Rolling.

At first I thought it was related to a very recent issue with the ament_cmake_vendor_package package, but it was fixed, so building from source should work (ament/ament_cmake#462).

@christophebedard
Copy link
Member

Huh, I re-triggered the job a few times over the last couple of days and now it passes.

This looks good to me, but I'll let @emersonknapp take a look at the last changes.

Also, could you just rebase on main? I think there were new changes.

@emersonknapp emersonknapp merged commit a026e1b into ros-tooling:main Jun 22, 2023
14 checks passed
@emersonknapp
Copy link
Contributor

@mllofriu thanks for your patience and the revision. I appreciate the work you've done!

anrp-tri pushed a commit to anrp-tri/topic_tools that referenced this pull request Jul 6, 2023
@mllofriu
Unit tests for all nodes (ros-tooling#61)
bfb39e7 
* Add a basic unit test for relay_node
* Add a test for drop
* Add a test for the throttle node
* Add a test for mux. Add a call to re-suscribe when the topic is switched

Signed-off-by: Martin Llofriu <mllofriu@gmail.com>

Signed-off-by: anrp <anrp@tri.global>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophebedard christophebedard christophebedard left review comments

@emersonknapp emersonknapp emersonknapp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mllofriu @christophebedard @emersonknapp