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

Location WiFi - twister+pytest test for native_sim and playback data #725

Closed
wants to merge 1 commit into from

Conversation

mniestroj
Copy link
Collaborator

No description provided.

@mniestroj mniestroj changed the base branch from main to location-wifi January 10, 2025 16:14
Copy link

github-actions bot commented Jan 10, 2025

Visit the preview URL for this PR (updated for commit 0eee67f):

https://golioth-firmware-sdk-doxygen-dev--pr725-location-wifi-w93yako9.web.app

(expires Sat, 18 Jan 2025 06:15:39 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: a9993e61697a3983f3479e468bcb0b616f9a0578

@mniestroj
Copy link
Collaborator Author

We get:

16:18:53.842:DEBUG:twister_harness.device.device_adapter: #: �[1;32muart:~$ �[m�[8D�[J[00:00:05.310,002] �[1;33m<wrn> location: Location not found�[0m
16:18:53.842:DEBUG:twister_harness.device.device_adapter: #: �[1;32muart:~$ �[m�[8D�[J[00:00:05.310,002] �[1;33m<wrn> golioth_coap_client_zephyr: CoAP Error: 4.04�[0m
16:18:53.922:DEBUG:twister_harness.device.device_adapter: #: �[1;32muart:~$ �[m�[8D�[J[00:00:05.380,001] �[1;33m<wrn> location: Location not found�[0m
16:18:53.922:DEBUG:twister_harness.device.device_adapter: #: �[1;32muart:~$ �[m�[8D�[J[00:00:05.380,001] �[1;33m<wrn> golioth_coap_client_zephyr: CoAP Error: 4.04�[0m
16:18:58.603:DEBUG:twister_harness.device.device_adapter: #: �[1;32muart:~$ �[m�[8D�[J[00:00:10.070,001] �[1;33m<wrn> location: Location not found�[0m
16:18:58.603:DEBUG:twister_harness.device.device_adapter: #: �[1;32muart:~$ �[m�[8D�[J[00:00:10.070,001] �[1;33m<wrn> golioth_coap_client_zephyr: CoAP Error: 4.04�[0m
16:19:03.602:DEBUG:twister_harness.device.device_adapter: #: �[1;32muart:~$ �[m�[8D�[J[00:00:15.070,001] �[1;33m<wrn> location: Location not found�[0m
16:19:03.603:DEBUG:twister_harness.device.device_adapter: #: �[1;32muart:~$ �[m�[8D�[J[00:00:15.070,001] �[1;33m<wrn> golioth_coap_client_zephyr: CoAP Error: 4.04�[0m

due to prod not supporting location yet.

Copy link
Contributor

@sam-golioth sam-golioth left a comment

Choose a reason for hiding this comment

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

Nice work @mniestroj! The failures in prod will be annoying but I think we should still merge this in.

Base automatically changed from location-wifi to main January 11, 2025 06:11
@mniestroj mniestroj force-pushed the location-wifi-twister branch from d03965f to 0eee67f Compare January 11, 2025 06:14
Rely on playback data to result in accurate location data. Wait for first 5
locations and match logged data with expected position.

Signed-off-by: Marcin Niestroj <[email protected]>
@mniestroj mniestroj force-pushed the location-wifi-twister branch from 0eee67f to 822fc1f Compare January 11, 2025 06:14
Copy link
Contributor

@sam-golioth sam-golioth left a comment

Choose a reason for hiding this comment

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

Oops! I thought I hit approve last week.

@mniestroj
Copy link
Collaborator Author

Nice work @mniestroj! The failures in prod will be annoying but I think we should still merge this in.

Merging this PR is in conflict with our strategy to increase number of "required" tests. Additionally there is already a test that prevents merging this PR:

image

Event if we would temporarily disable checks, then still all upcoming PRs will be blocked as that test won't pass. So we need to either wait for backend location service or disable native_sim tests as being required. The major benefit of having tests merged and running in the CI is to get feedback whenever something breaks... this won't be the case if we merge this one. It is great that we have the test already developed, but I don't think we gain anything by trying to merge early. So I propose to just wait and rebase once we are ready on the backend side.

@sam-golioth
Copy link
Contributor

The major benefit of having tests merged and running in the CI is to get feedback whenever something breaks... this won't be the case if we merge this one.

That's a great point. I'm good with waiting for the backend support 👍

@mniestroj
Copy link
Collaborator Author

Closing in favor of #730, which has combined cellular and wifi tests.

@mniestroj mniestroj closed this Jan 21, 2025
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.

3 participants