Updated QD to 1 #16

ahcorde · 2020-10-12T14:15:52Z

Added benchmark and test
Updated QD

Signed-off-by: ahcorde ahcorde@gmail.com

clalancette · 2020-10-12T14:21:43Z

CMakeLists.txt

+  list(APPEND extra_cmake_args "-DSPDLOG_BUILD_TESTS=ON")
  list(APPEND extra_cmake_args "-DSPDLOG_BUILD_EXAMPLE=OFF")
+  list(APPEND extra_cmake_args "-DSPDLOG_BUILD_BENCH=ON")


I'm don't think we should turn on the tests and benchmarks for the vendored package. First of all, are we going to run them? Second, even if we can run them, should we run them? I guess you are running them down below, so the answer to the first is "yes". But I don't really think we should run them, to be honest.

I believe the goal of running these tests is to detect regressions that could affect our code. It looks like we're pinning a specific commit, so maybe the risk that something breaks is very low. Is there a chance that something breaks due to factors external to the source code?

What's the concern with enabling tests, added time on our CI?

If one thing, I think the tests on this repository should respect the BUILD_TESTING variable.

We have done this for console_bridge_vendor to reach queality level 1. spdlog is running the test in Travis, but the benchmark tests are not running anywhere.

We are using a specific version of the code. Maybe it does not make sense to add any benchmark because the code it't not going to change

But at least we have all the benchmark in the same place.

Same logic applies for the API tests.

What's the concern with enabling tests, added time on our CI?

That's one concern. Another is that we can't build and run tests for every single thing we depend on. Some things just come from the system libraries, and we assume they are "good" enough.

We have done this for console_bridge_vendor to reach queality level 1.

I didn't realize that. I do think that it is in somewhat of a similar situation in that we shouldn't necessarily build the tests for it. But on the other hand, console_bridge is special in that we also maintain it, just not as a ROS 2 package.

In the end, if you think it adds value to run the tests here, then go ahead and keep it. I just wanted to question why we were doing this, and whether we should. But I agree with @chapulina that it should definitely respect BUILD_TESTING.

we can't build and run tests for every single thing we depend on

Yeah the hope is that the upstream projects themselves would be tested to a level that we consider satisfying for REP-2004. Unfortunately this isn't the case for spdlog, so one way of satisfying REP-2004 is by testing at least the parts that we care about. Not ideal, but I can't think of an alternative solution... 😕

Signed-off-by: ahcorde <ahcorde@gmail.com>

brawner

I think spdlog specific information should be moved to the SPDLOG_QUALITY_DECLARATION. Also, where are the benchmarks?

QUALITY_DECLARATION.md

Signed-off-by: ahcorde <ahcorde@gmail.com>

QUALITY_DECLARATION.md

SPDLOG_QUALITY_DECLARATION.md

brawner · 2020-10-13T18:34:14Z

Is this supposed to be adding benchmarks or just updating QDs?

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2020-10-14T07:10:44Z

Finally, we have decided not to include tests or benchmark. Update the title.

clalancette

One minor thing to fix, then this looks OK to me.

clalancette · 2020-10-14T12:51:11Z

SPDLOG_QUALITY_DECLARATION.md

+Though `spdlog` meets many of the criteria stated for software quality.
 As `spdlog` itself is not a ROS package, many of these gaps could be reconciled in the vendor package that is used to control `spdlog` distribution within ROS.
 Engagement with the upstream development team to define policies for versioning, API/ABI stability and enforcing peer review would lead to higher quality, and could increase the level category for `spdlog` in a subsequent review.


If we are justifying this as QL 1, we should just get rid of this whole paragraph.

chapulina

I think we can't bump spdlog to level 1 with the current text on items 4.ii and 4.iii. We need some explanation or some tests.

Finally, we have decided not to include tests or benchmark.

Could you document the rationale for that on the quality declaration? I think I missed something, I had understood we were going to add tests here to complement the ones upstream.

Signed-off-by: ahcorde <ahcorde@gmail.com>

SPDLOG_QUALITY_DECLARATION.md

Signed-off-by: ahcorde <ahcorde@gmail.com>

SPDLOG_QUALITY_DECLARATION.md

Signed-off-by: ahcorde <ahcorde@gmail.com>

…ndor into ahcorde/qd/update

clalancette

One more suggestion and one more question.

SPDLOG_QUALITY_DECLARATION.md

Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> Signed-off-by: ahcorde <ahcorde@gmail.com>

Signed-off-by: ahcorde <ahcorde@gmail.com>

chapulina

LGTM! I just have a couple of minor comments below.

SPDLOG_QUALITY_DECLARATION.md

brawner

Looks good pending @chapulina's suggestions.

clalancette

Looks good with @chapulina 's suggestions fixed.

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Updated Qd to 1 Signed-off-by: ahcorde <ahcorde@gmail.com> * update QD Signed-off-by: ahcorde <ahcorde@gmail.com> * Update Qd Signed-off-by: ahcorde <ahcorde@gmail.com> * Update section 4.i, 4.ii and 4.iii Signed-off-by: ahcorde <ahcorde@gmail.com> * updated QD Signed-off-by: ahcorde <ahcorde@gmail.com> * Update QD Signed-off-by: ahcorde <ahcorde@gmail.com> * Update qd Signed-off-by: ahcorde <ahcorde@gmail.com> * Update SPDLOG_QUALITY_DECLARATION.md Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> Signed-off-by: ahcorde <ahcorde@gmail.com> * Included used classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Added more details about coverage Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed link Signed-off-by: ahcorde <ahcorde@gmail.com> * update QD Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>

ahcorde added the documentation Improvements or additions to documentation label Oct 12, 2020

ahcorde requested review from brawner and chapulina October 12, 2020 14:15

ahcorde self-assigned this Oct 12, 2020

clalancette requested changes Oct 12, 2020

View reviewed changes

ahcorde force-pushed the ahcorde/qd/update branch from daa0fa8 to 4c0def8 Compare October 12, 2020 17:01

Updated Qd to 1

0998936

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde force-pushed the ahcorde/qd/update branch from 4c0def8 to 0998936 Compare October 12, 2020 17:15

brawner reviewed Oct 12, 2020

View reviewed changes

update QD

1f377ca

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from brawner October 13, 2020 08:05

chapulina reviewed Oct 13, 2020

View reviewed changes

QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

SPDLOG_QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

ahcorde changed the title ~~Updated QD to 1: Added tests and benchmark~~ Updated QD to 1 Oct 14, 2020

Update Qd

498197c

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested review from chapulina and clalancette October 14, 2020 07:10

clalancette requested changes Oct 14, 2020

View reviewed changes

chapulina suggested changes Oct 14, 2020

View reviewed changes

Update section 4.i, 4.ii and 4.iii

4b09ff1

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from chapulina October 15, 2020 08:10

updated QD

f262835

Signed-off-by: ahcorde <ahcorde@gmail.com>

chapulina suggested changes Oct 15, 2020

View reviewed changes

SPDLOG_QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

SPDLOG_QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

SPDLOG_QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

Update QD

f7a0402

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested review from chapulina and clalancette October 19, 2020 13:53

clalancette requested changes Oct 19, 2020

View reviewed changes

SPDLOG_QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

ahcorde added 2 commits October 20, 2020 10:17

Update qd

2807d2c

Signed-off-by: ahcorde <ahcorde@gmail.com>

Merge branch 'ahcorde/qd/update' of https://github.com/ros2/spdlog_ve…

d93c9c6

…ndor into ahcorde/qd/update

clalancette reviewed Oct 20, 2020

View reviewed changes

SPDLOG_QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

Update SPDLOG_QUALITY_DECLARATION.md

0f542d8

Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde force-pushed the ahcorde/qd/update branch from fba1884 to 0f542d8 Compare October 20, 2020 13:19

ahcorde added 2 commits October 20, 2020 20:06

Included used classes

8afd3d3

Signed-off-by: ahcorde <ahcorde@gmail.com>

Added more details about coverage

9745b4f

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from clalancette October 22, 2020 16:14

Fixed link

e152a4e

Signed-off-by: ahcorde <ahcorde@gmail.com>

chapulina approved these changes Oct 22, 2020

View reviewed changes

SPDLOG_QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

SPDLOG_QUALITY_DECLARATION.md Outdated Show resolved Hide resolved

brawner approved these changes Oct 22, 2020

View reviewed changes

clalancette approved these changes Oct 22, 2020

View reviewed changes

update QD

209530d

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde merged commit 4683c39 into master Oct 23, 2020

delete-merged-branch bot deleted the ahcorde/qd/update branch October 23, 2020 08:38

clalancette mentioned this pull request Oct 23, 2020

Progress toward Quality Level 1 #5

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated QD to 1 #16

Updated QD to 1 #16

ahcorde commented Oct 12, 2020

clalancette Oct 12, 2020

chapulina Oct 12, 2020

ahcorde Oct 12, 2020

clalancette Oct 12, 2020

chapulina Oct 12, 2020

brawner left a comment

brawner commented Oct 13, 2020

ahcorde commented Oct 14, 2020

clalancette left a comment

clalancette Oct 14, 2020

ahcorde Oct 15, 2020

chapulina left a comment

clalancette left a comment

chapulina left a comment

brawner left a comment

clalancette left a comment

Updated QD to 1 #16

Updated QD to 1 #16

Conversation

ahcorde commented Oct 12, 2020

clalancette Oct 12, 2020

Choose a reason for hiding this comment

chapulina Oct 12, 2020

Choose a reason for hiding this comment

ahcorde Oct 12, 2020

Choose a reason for hiding this comment

clalancette Oct 12, 2020

Choose a reason for hiding this comment

chapulina Oct 12, 2020

Choose a reason for hiding this comment

brawner left a comment

Choose a reason for hiding this comment

brawner commented Oct 13, 2020

ahcorde commented Oct 14, 2020

clalancette left a comment

Choose a reason for hiding this comment

clalancette Oct 14, 2020

Choose a reason for hiding this comment

ahcorde Oct 15, 2020

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

chapulina left a comment

Choose a reason for hiding this comment

brawner left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment