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

hil: ota: add test for observation after restart #506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

szczys
Copy link
Contributor

@szczys szczys commented May 29, 2024

Test that the firmware update observation is reestablished after a client restart.

resolves https://github.com/golioth/firmware-issue-tracker/issues/547

@szczys szczys force-pushed the szczys/ota_add_restart_test branch from be677d4 to 6d30539 Compare May 29, 2024 19:54
Copy link

github-actions bot commented May 29, 2024

Visit the preview URL for this PR (updated for commit 76e94fb):

https://golioth-firmware-sdk-doxygen-dev--pr506-szczys-ota-add-bpmcghzk.web.app

(expires Thu, 13 Jun 2024 14:40:00 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: a9993e61697a3983f3479e468bcb0b616f9a0578

@szczys szczys force-pushed the szczys/ota_add_restart_test branch 2 times, most recently from b45e641 to 70e89f7 Compare May 31, 2024 19:14
else if (golioth_sys_sem_take(restart_pending_sem, 0))
{
GLTH_LOGI(TAG, "Restart successful");
golioth_sys_sem_give(restart_test_begin_sem);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
golioth_sys_sem_give(restart_test_begin_sem);

We don't need to reset any semaphores for this test. We want each to run once and only once, to prevent restarting the test when the artifact is rolled back in the teardown phase of the test.

static void test_restart(void)
{

if (golioth_sys_sem_take(restart_test_begin_sem, 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to use semaphores inside of this function. We're not performing mutual exclusion between multiple threads, and we're not signaling from one thread to another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I've converted this to use a bool variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a lot more straightforward!

Test that the firmware update observation is reestablished after a client
restart.

Signed-off-by: Mike Szczys <mike@golioth.io>
@szczys szczys force-pushed the szczys/ota_add_restart_test branch from 70e89f7 to 76e94fb Compare June 6, 2024 14:39
@szczys szczys requested a review from sam-golioth June 6, 2024 14:40
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