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

Tests for mqtt interactions #992

Merged

Conversation

hasanheroglu
Copy link
Contributor

No description provided.

Signed-off-by: Hasan Eroglu <hasanheroglu@gmail.com>
Signed-off-by: Hasan eroglu <hasanheroglu@gmail.com>
Signed-off-by: Hasan eroglu <hasanheroglu@gmail.com>
Signed-off-by: Hasan eroglu <hasanheroglu@gmail.com>
Signed-off-by: Hasan eroglu <hasanheroglu@gmail.com>
Signed-off-by: Hasan eroglu <hasanheroglu@gmail.com>
@hasanheroglu
Copy link
Contributor Author

Tests were failing for handleAction because of input handling. So I used same logic as handlePropertyWrite for handleAction.

Signed-off-by: Hasan eroglu <hasanheroglu@gmail.com>
Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Some inputs:

  • The testing file is called mqtt-client-subscribe-test.integration.ts : the new tests are about writing and invoking actions. We have two options: rename the file to mqtt-client-test.integration.ts and use a describe to group the different tests OR move the new tests in a new file.
  • You tested for different input types which is great, but there is a lot of duplicated code. Can you try to refactor it to have less code duplicated?
  • I left some minor comments below about timeouts and failing.

@danielpeintner
Copy link
Member

Note: I noticed you merged in master which was not a good idea since the testing is broken at the moment but we are about to fix that, see #995
You may need to merge master once again when the PR is merged.

@danielpeintner
Copy link
Member

FYI: #995 has been merged and merging in master again should run the test workflow again.

hasanheroglu and others added 7 commits May 29, 2023 14:21
Co-authored-by: Jan Romann <jan.romann@uni-bremen.de>
Co-authored-by: Jan Romann <jan.romann@uni-bremen.de>
Signed-off-by: Hasan Eroglu <hasanheroglu@gmail.com>
…/thingweb.node-wot into tests-for-mqtt-interactions
…/thingweb.node-wot into tests-for-mqtt-interactions
Signed-off-by: Hasan Eroglu <hasanheroglu@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented May 29, 2023

Codecov Report

Merging #992 (48e615f) into master (c4312fe) will increase coverage by 1.15%.
The diff coverage is 50.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
+ Coverage   74.64%   75.79%   +1.15%     
==========================================
  Files          72       72              
  Lines       14882    14856      -26     
  Branches     1412     1429      +17     
==========================================
+ Hits        11108    11260     +152     
+ Misses       3741     3563     -178     
  Partials       33       33              
Impacted Files Coverage Δ
packages/binding-mqtt/src/mqtt-broker-server.ts 77.01% <50.00%> (+31.47%) ⬆️

... and 5 files with indirect coverage changes

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Much better now! thank you! I think we can move on and fix the details in the future.

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

It's good to see code coverage improve 👍

@danielpeintner danielpeintner merged commit 859f1ce into eclipse-thingweb:master May 30, 2023
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