Skip to content

test(binding-coap): await all calls of coapServer.stop() #1094

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
Merged
relu91 merged 4 commits into from
Sep 21, 2023
Merged

test(binding-coap): await all calls of coapServer.stop() #1094

relu91 merged 4 commits into from
Sep 21, 2023

Conversation

@JKRhb
Copy link
Member

@JKRhb JKRhb commented Sep 21, 2023 •
edited
Loading

In the context of #1086 and #1077, I noticed that in the CoAP tests, not all invocations of coapServer.stop() are actually explicitly being awaited which might cause the CI to hang if there is an unexpected error.

This PR makes a couple of minor adjustments to improve the cleanup of resources and fix this potential problem (although I am not sure to which extent it is actually a problem to begin).

@JKRhb JKRhb changed the title test(binding-coap): await´ all calls of coapServer.stop()` test(binding-coap): await all calls of coapServer.stop() Sep 21, 2023
@codecov
Copy link

codecov bot commented Sep 21, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (95ea950) 75.32% compared to head (7a3538a) 75.32%.
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1094   +/-   ##
=======================================
  Coverage   75.32%   75.32%           
=======================================
  Files          80       80           
  Lines       16057    16057           
  Branches     1503     1503           
=======================================
  Hits        12095    12095           
  Misses       3923     3923           
  Partials       39       39

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JKRhb JKRhb marked this pull request as ready for review September 21, 2023 11:27
@JKRhb
Copy link
Member Author

JKRhb commented Sep 21, 2023

As I remembered that using both EventEmitter and async can be a bit problematic, I now wrapped the test code in question into Promises, which also had the benefit of slightly better performance, if I am not mistaken :) If your approval stands, then we could merge this one :)

@relu91
Copy link
Member

relu91 commented Sep 21, 2023

As I remembered that using both EventEmitter and async can be a bit problematic, I now wrapped the test code in question into Promises, which also had the benefit of slightly better performance, if I am not mistaken :) If your approval stands, then we could merge this one :)

Just saw this video yesterday 😄

@relu91 relu91 merged commit 27970b0 into eclipse-thingweb:master Sep 21, 2023
@JKRhb JKRhb deleted the coap-server-await branch September 21, 2023 16:42
@JKRhb
Copy link
Member Author

JKRhb commented Sep 21, 2023

As I remembered that using both EventEmitter and async can be a bit problematic, I now wrapped the test code in question into Promises, which also had the benefit of slightly better performance, if I am not mistaken :) If your approval stands, then we could merge this one :)

Just saw this video yesterday 😄

:D I also had that one in mind ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@relu91 relu91 relu91 approved these changes

@danielpeintner danielpeintner danielpeintner 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.

3 participants

@JKRhb @relu91 @danielpeintner