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

Wait for node discovery in test_generate_policy. #262

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Apr 14, 2021

Replacement for #260. Only wait when daemon is in use (if not, CLI will use a different node).

CI up to sros2:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Only when daemon is in use (if not, CLI will use a different node).

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic requested a review from clalancette April 14, 2021 23:09
@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #262 (75e4005) into master (ac0795c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #262   +/-   ##
=======================================
  Coverage   77.25%   77.25%           
=======================================
  Files          25       25           
  Lines         664      664           
  Branches       55       55           
=======================================
  Hits          513      513           
  Misses        131      131           
  Partials       20       20           
Flag Coverage Δ
unittests 77.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../hfyv6abjie8/sros2/sros2/sros2/command/security.py
ros_ws/src/hfyv6abjie8/sros2/sros2/sros2/errors.py
...hfyv6abjie8/sros2/sros2/sros2/keystore/__init__.py
...fyv6abjie8/sros2/sros2/sros2/keystore/_keystore.py
...v6abjie8/sros2/sros2/sros2/verb/generate_policy.py
...v6abjie8/sros2/sros2/sros2/keystore/_permission.py
...yv6abjie8/sros2/sros2/sros2/verb/create_enclave.py
...jie8/sros2/sros2/sros2/policy/defaults/__init__.py
...src/hfyv6abjie8/sros2/sros2/sros2/verb/__init__.py
...c/hfyv6abjie8/sros2/sros2/sros2/policy/__init__.py
... and 40 more

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 ac0795c...75e4005. Read the comment docs.

@hidmic
Copy link
Contributor Author

hidmic commented Apr 15, 2021

@ros-pull-request-builder retest this please.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks much better to me, thanks. And CI seems to be happy with it as well.

@hidmic
Copy link
Contributor Author

hidmic commented Apr 15, 2021

Cool, thanks for the reviews! Does anybody have the bits to merge now? 😅

@clalancette
Copy link
Contributor

@SidFaber If you get a chance, can you review/merge this?

Copy link

@SidFaber SidFaber left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@SidFaber
Copy link

@kyrofa or @mikaelarguedas can you merge?

@kyrofa kyrofa merged commit e5e90f0 into master Apr 15, 2021
@kyrofa kyrofa deleted the hidmic/wait-for-node-discovery branch April 15, 2021 23:03
@mikaelarguedas
Copy link
Member

How was it tested that this addresses the issue ?
Can we confirm which test failure this is addressing ? which platform / architecture combination?

Looking at CI for this PR, it's not repeating any test so I'm not sure we can use that as reference to determine it reduces flakiness of a given test.

Comparing nightly CI referenced in the original issue:
https://ci.ros2.org/view/nightly/job/nightly_osx_repeated/lastCompletedBuild/testReport/sros2.test.sros2.commands.security.verbs/test_generate_policy/test_generate_policy/
And the one after this was merged:
https://ci.ros2.org/view/nightly/job/nightly_osx_repeated/2315/testReport/sros2.test.sros2.commands.security.verbs/test_generate_policy/test_generate_policy/

So maybe this PR did not have an effect ?

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.

6 participants