Skip to content

Refactor set_params helper to reduce tests runtime#1127

Merged
Turnlings merged 5 commits intomainfrom
1117-investigate-long-test
Mar 27, 2026
Merged

Refactor set_params helper to reduce tests runtime#1127
Turnlings merged 5 commits intomainfrom
1117-investigate-long-test

Conversation

@Turnlings
Copy link
Copy Markdown
Contributor

@Turnlings Turnlings commented Mar 25, 2026

Significantly reduces test runtime by refactoring function used in setup that used to take 120s.

Reduces a python tests run on Github from about 5m 20s to about 2m 40s. So pretty much exactly in half.

@Turnlings Turnlings linked an issue Mar 25, 2026 that may be closed by this pull request
@Turnlings
Copy link
Copy Markdown
Contributor Author

@1Blademaster the broken tests seem to be because it is returning the old values that you changed last week in #1083 ?
I can't work out what needs to be different to get it in line with the new values.

Like this is the diff of why the test is failing

image

And this is the diff of the changes you made last week

image

@1Blademaster
Copy link
Copy Markdown
Member

@Turnlings that's interesting. I'm honestly not sure because I had issues with it as well, but I did eventually get it working and pushed to docker hub so the action runner should pick up the change. Does it pass locally out of curiosity?

@Turnlings
Copy link
Copy Markdown
Contributor Author

@1Blademaster Nope. The exact same errors locally too, where I'm using the :latest image.

@1Blademaster
Copy link
Copy Markdown
Member

I'm kinda lost tbh, it's very annoying error.

@Turnlings
Copy link
Copy Markdown
Contributor Author

@1Blademaster I get the same errors just running the tests on the main branch locally too.
I'll ask to see if any of the others have the same issue or if its a me problem.

@1Blademaster
Copy link
Copy Markdown
Member

image

The x, y and z in the test being 0 is not correct; something must be going wrong somewhere I think.

@Turnlings Turnlings changed the title [WIP] Potential faster solution, see if stable on action runner Refactor set_params helper to reduce tests runtime Mar 26, 2026
@Turnlings Turnlings marked this pull request as ready for review March 26, 2026 21:06
Copilot AI review requested due to automatic review settings March 26, 2026 21:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the radio/tests/helpers.py:set_params() test helper to reduce overall test runtime by batching MAVLink parameter writes and consolidating retry/ack handling during session-scoped test setup.

Changes:

  • Replaces per-parameter retry loops with a batched “send all pending params” approach.
  • Introduces pending/retry tracking maps and a buffer-drain helper to reduce stale ACK interference.
  • Updates timing behavior (new timeout loop + end-of-function settle sleep).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread radio/tests/helpers.py Outdated
Comment thread radio/tests/helpers.py Outdated
Comment thread radio/tests/helpers.py
Comment thread radio/tests/helpers.py
@Turnlings
Copy link
Copy Markdown
Contributor Author

Turnlings commented Mar 26, 2026

Yeah turned out that it was a me issue...

SITL Simulator clearly doesn't handle new docker images the way I thought it would, so I was on the wrong version and hunting for the wrong bug.

Fix for this bug was it just needed some time to settle for the state to line up (Settling time is much less than the time it took before so still saving a lot of time)

SITL thing raises the issue of what behaviour we expect for that when you update the docker image. Probably a warning label and button to update? (Don't want it downloading 4GB out of the blue, also should it delete the old one?) Anyway lots of questions for a new ticket...

I've requested a review, but in the meantine I'm going to run the tests a lot to make sure that it is definitely stable.

@Turnlings Turnlings requested a review from a team March 26, 2026 21:31
@Turnlings Turnlings self-assigned this Mar 26, 2026
Copy link
Copy Markdown
Member

@1Blademaster 1Blademaster left a comment

Choose a reason for hiding this comment

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

Seems fine to me from reading through 👍

@Turnlings Turnlings merged commit 0f2cd0e into main Mar 27, 2026
15 checks passed
@Turnlings Turnlings deleted the 1117-investigate-long-test branch March 27, 2026 13:01
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.

[TASK] Investigate long test

3 participants