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(core): don't fetch full DebugLinkState by default #4568

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

romanz
Copy link
Contributor

@romanz romanz commented Feb 2, 2025

In case the main workflow is restarting after a DebugLinkDecision, sending the response of DebugLinkGetState may get interrupted (see #4401).

Let's make the full DebugLinkGetState fetching explicit, to avoid the "restart" race condition.

@romanz romanz self-assigned this Feb 2, 2025
@romanz romanz linked an issue Feb 2, 2025 that may be closed by this pull request
Copy link

github-actions bot commented Feb 2, 2025

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@romanz romanz force-pushed the romanz/dont-fetch-debuglink-state branch 2 times, most recently from 2140bbe to 116377b Compare February 3, 2025 10:20
@matejcik
Copy link
Contributor

matejcik commented Feb 3, 2025

no, let's not do that again :( having to propagate return_layout=True everywhere is a major pain and it was super nice to be rid of it after GFL
if we go this route, I'd strongly prefer to never return the layout, and forcing caller to explicitly call read_layout() if they need it.

on a separate note, this breaks the "read text representation of screens" feature. but i don't think anyone is actually using it?

another option is to always return the layout, but make the LayoutContent lazy. you'd have to track both debuglink and wirelink and invalidate the lazy-load -- raise an exception if someone tries to access it after any other messages have been exchanged, because any wirelink traffic can cause the layout to change, so it's no longer the "return value of that particular decision"

@romanz
Copy link
Contributor Author

romanz commented Feb 3, 2025

if we go this route, I'd strongly prefer to never return the layout, and forcing caller to explicitly call read_layout() if they need it.

Sounds good, I'll change the code to separate sending DebugLinkDecision and fetching the DebugLinkState into separate methods.

this breaks the "read text representation of screens" feature. but i don't think anyone is actually using it?

IIUC, the text representation is collected into tests/ui_tests/reports/test/screen_text.txt when --record-text-layout is specified, e.g.:

pytest -vvv ../tests/device_tests/binance/test_get_address.py --record-text-layout
T3B1_en-device_tests-binance-test_get_address.py::test_binance_get_address[m-44h-714h-0h-0-0-bnb1hgm0p7khfk85zpz-68e2cb5a

T3B1_en-device_tests-binance-test_get_address.py::test_binance_get_address[m-44h-714h-0h-0-1-bnb1egswqkszzfc2uq7-1adfb691

T3B1_en-device_tests-binance-test_get_address.py::test_binance_get_address_chunkify_details[m-44h-714h-0h-0-0-bn-59d4996f
        Account:
        BNB #1
        Derivation path:
        m/44'/714'/0'/0/0
        ********************
        -, -, -
        ////////////////////////////////////////////////////////////////////////////////
        bnb1hgm0p7khfk85zpz5v0j8wnej3a90w709vhkdfu
        ********************
        -, -, -
        ////////////////////////////////////////////////////////////////////////////////

T3B1_en-device_tests-binance-test_get_address.py::test_binance_get_address_chunkify_details[m-44h-714h-0h-0-1-bn-c9025900
        Account:
        BNB #1
        Derivation path:
        m/44'/714'/0'/0/1
        ********************
        -, -, -
        ////////////////////////////////////////////////////////////////////////////////
        bnb1egswqkszzfc2uq78zjslc6u2uky4pw46x4rstd
        ********************
        -, -, -
        ////////////////////////////////////////////////////////////////////////////////

Not sure if there is any other tool that depends on it...

@romanz romanz force-pushed the romanz/dont-fetch-debuglink-state branch 4 times, most recently from 933fffe to 1c18a26 Compare February 4, 2025 16:55
@romanz romanz added core Trezor Core firmware. Runs on Trezor Model T and T2B1. ci Continuous Integration (CI) related tests Automated integration tests python Pull requests that update Python code labels Feb 4, 2025
@romanz romanz marked this pull request as ready for review February 4, 2025 17:53
@romanz romanz requested a review from matejcik as a code owner February 4, 2025 17:53
@romanz
Copy link
Contributor Author

romanz commented Feb 5, 2025

Manually tested on TS5: #4401 (comment)

@romanz romanz force-pushed the romanz/dont-fetch-debuglink-state branch from 1596c8c to abb1d59 Compare February 5, 2025 20:42
@romanz romanz changed the title test(core): don't fetch DebugLinkState by default test(core): don't fetch full DebugLinkState by default Feb 6, 2025
@romanz romanz requested a review from matejcik February 6, 2025 12:03
python/src/trezorlib/debuglink.py Show resolved Hide resolved
tests/click_tests/common.py Outdated Show resolved Hide resolved
@romanz romanz force-pushed the romanz/dont-fetch-debuglink-state branch from 26ab041 to a9177c6 Compare February 7, 2025 09:09
In case the main workflow is restarting after a `DebugLinkDecision`,
sending the response of `DebugLinkGetState` may get interrupted.

We are making the state fetching explicit, in order to avoid the
"restart" race condition (as described in #4401).

Following the above change, text-based layout recording is removed.

[no changelog]
@romanz romanz force-pushed the romanz/dont-fetch-debuglink-state branch from a9177c6 to 693283f Compare February 7, 2025 09:30
@romanz
Copy link
Contributor Author

romanz commented Feb 7, 2025

Thanks! I have squashed, rebased and updated commit description.

@romanz romanz merged commit 061e712 into main Feb 7, 2025
94 checks passed
@romanz romanz deleted the romanz/dont-fetch-debuglink-state branch February 7, 2025 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration (CI) related core Trezor Core firmware. Runs on Trezor Model T and T2B1. python Pull requests that update Python code tests Automated integration tests
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

Debuglink response can get cut off if a workflow ends
2 participants