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

Grdddj/core unit tests cleanup #2196

Merged
merged 10 commits into from
Feb 22, 2024
Merged

Grdddj/core unit tests cleanup #2196

merged 10 commits into from
Feb 22, 2024

Conversation

grdddj
Copy link
Contributor

@grdddj grdddj commented Mar 29, 2022

Quick code and style improvements of unit-tests connected with #2193

We might not include the black changes, which have a very big diff - but they will decrease any future diffs when modifying the test cases

@grdddj grdddj requested a review from matejcik as a code owner March 29, 2022 10:29
@grdddj grdddj linked an issue May 5, 2022 that may be closed by this pull request
@grdddj grdddj force-pushed the grdddj/core_unit_tests_cleanup branch from 9340c34 to ba2281a Compare June 28, 2023 11:29
@grdddj
Copy link
Contributor Author

grdddj commented Jun 28, 2023

Force-pushed new changes done on the latest master.

Also, in addition to styling/code quality changes, allowing unit-tests for TR and adding this test into CI

@Hannsek Hannsek requested review from obrusvit and removed request for matejcik January 25, 2024 13:17
@obrusvit obrusvit force-pushed the grdddj/core_unit_tests_cleanup branch from ba2281a to 781e4b1 Compare January 26, 2024 11:27
@obrusvit
Copy link
Contributor

obrusvit commented Jan 26, 2024

I rebased the branch on current main and re-run black and isort.

In order to add core/tests into style target, multiple more issues must be addressed. The most numerous warnings are:

  • F405 - star import (originating from from common import *)
  • E501 - long lines
  • I900 - 'apps' not listed as a requirement
  • short variables, etc.

This PR also adds build and test targets specific for TS3.

If CI checks pass, I suggest to merge to partially improve the style of tests nad fix the issues later.

@obrusvit obrusvit force-pushed the grdddj/core_unit_tests_cleanup branch from 781e4b1 to d25139a Compare January 26, 2024 14:42
Copy link
Contributor

@obrusvit obrusvit left a comment

Choose a reason for hiding this comment

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

With the limitations highlighted in my previous comment, fine by me.

@matejcik
Copy link
Contributor

matejcik commented Feb 2, 2024

please also add this to Github Actions config. the gitlab CI is kind-of deprecated

@obrusvit
Copy link
Contributor

obrusvit commented Feb 4, 2024

I added T2B1 for python, rust and rust-client unit tests.

As per #2724 also persistence and upgrade tests should be added. However, those seem not prepared for T2B1 so I consider it OOS for this PR.

@matejcik
Copy link
Contributor

matejcik commented Feb 5, 2024

rust-client is failing:

---- tests::test_emulator_features stdout ----
thread 'tests::test_emulator_features' panicked at 'assertion failed: `(left == right)`
  left: `"Safe 3"`,
 right: `"T"`', src/lib.rs:174:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

this is super uninteresting, let's just disable rust-client on T2B1 for now

firmware clippy tests are also failing:

error: this arithmetic operation will overflow
   --> src/ui/model_tt/component/homescreen/render.rs:147:30
    |
147 |                 &mut workbuf[2 * HOMESCREEN_TOIF_X_OFFSET
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to compute `2_usize * 18446744073[70](https://github.com/trezor/trezor-firmware/actions/runs/7775914738/job/21202488123?pr=2196#step:5:71)9551608_usize`, which would overflow
    |
    = note: `#[deny(arithmetic_overflow)]` on by default

i've seen this before but I don't remember what it's about, investigate pls?

@matejcik
Copy link
Contributor

matejcik commented Feb 5, 2024

....on second thought, maybe first rebase on current main (feel free to squash all the fixups now) and see if the error goes away

@obrusvit obrusvit force-pushed the grdddj/core_unit_tests_cleanup branch from d34a78e to 101043a Compare February 5, 2024 15:08
@obrusvit
Copy link
Contributor

obrusvit commented Feb 5, 2024

rebased (with --autosquash) on top of current main. Let's wait for the result of rust tests, then I investigate.

@obrusvit obrusvit force-pushed the grdddj/core_unit_tests_cleanup branch from 101043a to 30293c7 Compare February 5, 2024 16:33
@obrusvit
Copy link
Contributor

obrusvit commented Feb 5, 2024

Fixed the uninteresting part (trezorctl).

The 2nd one (rust unit tests) still fails.
It's probably something with TREZOR_MODEL, you can see clippy reaching to src/ui/model_tt/component/homescreen/render.rs: (model T). However, if I build firmware for TREZOR_MODEL=R:

  • make clippy works on my machine
  • make test_rust fails
  • TREZOR_MODEL=R make test_rust works fine

Btw. this line is hellish 😆 --features model_t$(shell echo $(TREZOR_MODEL) | tr "TR" "tr")

@grdddj grdddj requested a review from prusnak as a code owner February 7, 2024 15:12
@matejcik
Copy link
Contributor

I briefly looked into this error and my suspicion is that it's a clippy version problem. I can randomly reproduce it locally when I manage to tell Rust to ignore my local versions of clippy and use the one from nix-shell, but even nix-shell --pure does not block access to my non-nix clippy so 🤷‍♀️

let's mark the test as allowed-to-fail, and see if #3337 helps

@obrusvit obrusvit force-pushed the grdddj/core_unit_tests_cleanup branch from 4859637 to 609d4d5 Compare February 22, 2024 09:46
@obrusvit
Copy link
Contributor

I rebased and excluded T2B1 from core_unit_rust_test job with explanatory comment.

Apparently, GH Actions currently don't have "allow failure" feature..? actions/runner#2347

@obrusvit obrusvit force-pushed the grdddj/core_unit_tests_cleanup branch from 609d4d5 to c52ae73 Compare February 22, 2024 09:48
grdddj and others added 6 commits February 22, 2024 11:40
isort set to skip the first necessary "from common import *" line. A
better solution would be to get rid of the need of this import in the
future.

[no changelog]
Disable some tests of unsupported coins for TS3.

[no changelog]
@obrusvit obrusvit force-pushed the grdddj/core_unit_tests_cleanup branch from c52ae73 to 61219e4 Compare February 22, 2024 10:41
@obrusvit
Copy link
Contributor

rebased again because of omitted trailing whitespace in core.yaml

@matejcik
Copy link
Contributor

OK then, ship it

@obrusvit obrusvit merged commit e1f696b into main Feb 22, 2024
63 checks passed
@obrusvit obrusvit deleted the grdddj/core_unit_tests_cleanup branch February 22, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Clean core unit tests
3 participants