ros2 / sros2

tools to generate and distribute keys for SROS 2
Apache License 2.0
90 stars 45 forks source link

permission api: use _keystore.get_* functions #194

Closed kyrofa closed 4 years ago

kyrofa commented 4 years ago

Proof that we're still completely missing test coverage of some verbs (logged as #193) :cry: . Added some here.

codecov[bot] commented 4 years ago

Codecov Report

Merging #194 into master will increase coverage by 4.95%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
+ Coverage   55.38%   60.34%   +4.95%     
==========================================
  Files          17       17              
  Lines         585      585              
  Branches       52       52              
==========================================
+ Hits          324      353      +29     
+ Misses        247      218      -29     
  Partials       14       14              
Flag Coverage Δ
#unittests 60.34% <100.00%> (+4.95%) :arrow_up:
Impacted Files Coverage Δ
sros2/sros2/api/_permission.py 92.30% <100.00%> (+30.76%) :arrow_up:
sros2/sros2/verb/create_permission.py 68.00% <0.00%> (+68.00%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1b41a2a...e7156ce. Read the comment docs.

kyrofa commented 4 years ago

It changes it to a... segfault? What the heck? Any chance that's Focal/SSL-related? I just ran ros2 security create_permission for the basic talker/listener example and got AttributeErrors without this patch, and success with it (on Bionic).

mikaelarguedas commented 4 years ago

I just ran ros2 security create_permission for the basic talker/listener example and got AttributeErrors without this patch, and success with it.

I confirmed I have a segfault on Focal with the repo state before all the refactoring so not related to this PR. Not great but at least clears this ...

As mentionned offline, committing a simple test along this PR would be great, full coverage can be done at another time.

kyrofa commented 4 years ago

committing a simple test along this PR would be great, full coverage can be done at another time.

Done. Couldn't let codecov own me like that. I'm afraid that I rebased instead of merged though, I'm sorry. At least the diff is tiny.

ruffsl commented 4 years ago

Later on, it might be easier to just compare the permission.xml tree to a reference xml file, and just pre-processes steps to remove/sanitize known diffs, like <validity>, or <domains> matches the ros domain env, or exceptional rules for logging and discovery. Once you filter out those tags, the rest of the tree for the _test_identity grant name your checking should be the same when pretty printed.

https://github.com/ros2/sros2/blob/1984821f52d23c245452275c0f7820461cd590bc/sros2/test/policies/permissions/talker_listener/permissions.xml#L2-L4