-
Notifications
You must be signed in to change notification settings - Fork 10
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for the AMCL node #194
Conversation
f9d7161
to
2e2ff77
Compare
0d59e14
to
3787643
Compare
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM for some unit testing complementing benchmarks. Nice work @nahueespinosa !
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
3787643
to
094d7a0
Compare
@hidmic Thanks for the review! The comments have been addressed. |
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
Proposed changes
Related to #101, this adds tests for the AMCL node up to 95% line coverage. It also fixes a couple of issues with lifecycle management and removes the read_only property for parameters that can change at runtime.
Type of change
Checklist
Additional comments
This is still work-in-progress.