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

test: check publishing of telemetry data whilst mapper is down #3187

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

gligorisaev
Copy link
Contributor

@gligorisaev gligorisaev commented Oct 15, 2024

This PR adds a new test case to validate the behavior of ThinEdge.io when the tedge-mapper-c8y service is down, and events are published during the downtime. The test ensures that events sent via MQTT while the mapper is offline are published to Cumulocity when the service is restored.

Test Flow:

  1. Stop the tedge-mapper-c8y service.
  2. Publish an MQTT event with a random event type and a test message.
  3. Restart the tedge-mapper-c8y service.
  4. Assert that the event is correctly received and processed by Cumulocity, verifying both the event type and message content.

Proposed changes

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3185

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@gligorisaev gligorisaev requested a review from a team as a code owner October 15, 2024 08:01
@gligorisaev gligorisaev changed the title System Test for the mosquitto bug causes mqtt session persistence to be non-functional est: for the mosquitto bug causes mqtt session persistence to be non-functional Oct 15, 2024
@gligorisaev gligorisaev changed the title est: for the mosquitto bug causes mqtt session persistence to be non-functional test: for the mosquitto bug causes mqtt session persistence to be non-functional Oct 15, 2024
@gligorisaev gligorisaev changed the title test: for the mosquitto bug causes mqtt session persistence to be non-functional test: Mosquitto bug causes mqtt session persistence to be non-functional Oct 15, 2024
Copy link
Contributor

github-actions bot commented Oct 17, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
509 0 2 509 100 1h29m38.842096999s

@reubenmiller
Copy link
Contributor

@gligorisaev some general comments/improvements:

  • Please use fixup commits when addressing comments explicitly related changes/problems to previously committed code (this helps keep the git commit history clean, as your second commit does not add any valuable information)

  • The naming of the test suite and the filename does not suit the contract that we want to provide to users. e.g. we shouldn't be explicitly checking for the mosquitto bug, we should be checking that thin-edge.io does not lose data if a client publishes qos1 or 2 messages whilst the mapper is down (notice the commit that I pushed 54b7b0c)

  • Prefer using Test Setup over Suite Setup unless you have a really good reason to use Suite Setup

  • Use unique identifiers when publishing test data. This helps reduce unexpected mistakes/behaviour if whatever assertion that you're doing looks beyond the expected scope (e.g. it looks at all events in the platform instead of events for the given device)...this is the reason why your test is passing (when it shouldn't), and my test is failing (as it should)

  • Use existing keywords where possible, e.g. prefer using Stop Service instead of manually running sudo systemctl stop .... The existing library keywords help the tests to be abstracted so that we can support other service managers using the same tests (e.g. SysVinit etc.)

  • Prefix the library on specific keywords where the meaning matters, for example Cumulocity.Device Should Have Event/s is more clear where the events are meant to be located (e.g. in Cumulocity and not on the local MQTT broker)

  • Keep the PR description up to date to reflect the purpose of the changes. The purpose of the ticket is to add coverage for offline publishing (not to test a mosquitto bug)...the mosquitto bug aspect just was the motivation to add such a test...if we had had the system test prior to upgrading to mosquitto 2.0.18, the test would of shown us that 2.0.18 has an issue (ahead of time before we used it in all of our images; system test image, rugpi image, yocto image etc.)

@reubenmiller
Copy link
Contributor

@gligorisaev

  • Always put the linked ticket under the Paste Link to the issue section. You can also mention the ticket link in the description as well (if provides additional context), however having the links all in the same place makes it easier to read all the PRs and the content is normalized.

Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

@gligorisaev Can you remove the following commits from the PR as they have been superseded by 54b7b0c:

And can you please update the PR description to suite (as per comments):

Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

@gligorisaev Thanks for dropping the two other commits, but now the commit I pushed yesterday to switch back to using mosquitto 2.0.11 is missing which means the test is failing (as the system tests are still using the bugging mosquitto 2.0.18 version)

Reverting back to mosquitto 2.0.11 due to a persistent session bug which affects thin-edge.io's ability to process any messages which are published locally whilst the mapper is down

Signed-off-by: Reuben Miller <[email protected]>
@reubenmiller reubenmiller changed the title test: Mosquitto bug causes mqtt session persistence to be non-functional test: check offline publishing of telemetry data Oct 18, 2024
@reubenmiller reubenmiller added theme:mqtt Theme: mqtt and mosquitto related topics theme:testing labels Oct 18, 2024
@reubenmiller reubenmiller self-requested a review October 18, 2024 11:58
reubenmiller

This comment was marked as duplicate.

@reubenmiller reubenmiller dismissed their stale review October 18, 2024 11:59

The new commits are authored by myself, so I shouldn't be reviewing it

@reubenmiller reubenmiller changed the title test: check offline publishing of telemetry data test: check publishing of telemetry data whilst mapper is down Oct 18, 2024
@reubenmiller
Copy link
Contributor

@gligorisaev Thanks for dropping the two other commits, but now the commit I pushed yesterday to switch back to using mosquitto 2.0.11 is missing which means the test is failing (as the system tests are still using the bugging mosquitto 2.0.18 version)

I did rebase and choose drop of my commits only (made also screenshot of it)

Then something went very wrong (maybe you didn't do a fetch and pull prior to rebasing). But anyway I've forced pushed the changes that I still had locally.

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved

@reubenmiller reubenmiller added this pull request to the merge queue Oct 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 18, 2024
@reubenmiller reubenmiller added this pull request to the merge queue Oct 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 18, 2024
@reubenmiller reubenmiller added this pull request to the merge queue Oct 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 18, 2024
@reubenmiller reubenmiller added this pull request to the merge queue Oct 18, 2024
Merged via the queue into thin-edge:main with commit 1011e4b Oct 18, 2024
31 checks passed
@gligorisaev gligorisaev deleted the mosquitto_bug branch October 24, 2024 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:mqtt Theme: mqtt and mosquitto related topics theme:testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants