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

Add integration tests #7

Merged

Conversation

ValMobBIllich
Copy link
Contributor

Adds a first integration test for the unencrypted publish/ subscribe case. Should more or less follow the conventions from the integration tests in the zenoh transport repo.

The test pipeline now includes docker composing a mosquitto broker and only runs the tests after the broker is reachable.

The encrypted case and possibly some tests for error cases are coming as soon as I can figure out how to configure the broker properly... Either in this PR or in a later one.

Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Looks good! Will you plan to bring RPC and Notification testing into this PR too?

@@ -20,8 +20,9 @@ on:
paths:
- "src/**"
- "Cargo.*"
workflow_call:
workflow_dispatch:
- "tests/**"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -19,7 +19,11 @@ cargo test

### Running the Examples

1. Start an MQTT broker (e.g. mosquitto)
1. Start an MQTT broker or use the included Mosquitto broker:
```bash
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

mosquitto:
image: eclipse-mosquitto:2.0
volumes:
# read-only prevents the container changing file owners on the host
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

tests/publish_subscribe.rs Outdated Show resolved Hide resolved
@ValMobBIllich
Copy link
Contributor Author

Looks good! Will you plan to bring RPC and Notification testing into this PR too?

Lets see. It looks like I can copypaste a lot from zenoh but first Ill try to understand whats going on there a bit.

Copy link
Contributor

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

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

Thanks @ValMobBIllich! A good starting point. LGTM

Reminder to us that we should do #9 to improve integration test coverage.

Could you now author a meaningful commit history so I can perform a merge commit?

@PLeVasseur PLeVasseur merged commit b519324 into eclipse-uprotocol:main Nov 14, 2024
4 checks passed
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.

2 participants