scripts: twister: Fix Twister Unit tests on Windows #74651
Conversation
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
6 times, most recently
from
dd3865f to
c4cc343
Compare
zephyrbot
requested review from
KamilxPaszkiet,
PerMac,
gchwier,
golowanow,
gopiotr,
hakehuang and
nashif
LukaszMrugala
requested review from
arikgreen,
gbarkadiusz,
mdubielx and
pkunieck
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
from
f7d1d0a to
31bce87
Compare
There was a problem hiding this comment.
Problem: Paths were deemed the main culprit. Windows has a limit of 260 character file paths by default.
Problem: Windows and Linux have a limit of 260 character filepath element.
So you are basically trying to solve the same problem that --short-build-path (see #41930) already solves, but in a different way.
Does this actually work? Because Ninja and Zephyr SDK Windows toolchain binaries are not exactly long-path friendly, relying on the "ANSI version" of the I/O functions.
There was a problem hiding this comment.
I've had to make some fixes before answering you (Zephyr test failures). Currently, this change gives us a PASS on all Zephyr tests and all Unit Tests relevant for Windows (So without i.a. jobserver and QEMUHandler). I've tested that on a private CI.
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
2 times, most recently
from
cc8a8d5 to
9d9fa48
Compare
|
It's hard for me to evaluate this PR. I don't understand the need for all this changes to paths and in particular the new TPath class. IMO TPath adds another layer which can complicate simple operations that were done with paths. I am aware about the first 2 bullet points (char limit on Windows) but I was sure we managed to resolve it with introduction of --short-build-path. In Nordic we have a CI which is building tests/samples on Windows with twister and it works. I think I also had to add --no-detailed-test-id at some point to remove paths from test ids but cannot recall what was the root cause for this. |
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
from
083ed1b to
d76f691
Compare
LukaszMrugala
changed the title
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
3 times, most recently
from
ae275a7 to
cb24d66
Compare
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
3 times, most recently
from
03a2e2b to
fd746d8
Compare
|
I have gotten back to this PR now and am able to remove the DNM label. While Twister itself can currently be run on Windows with the use of specific Twister flag, how can someone making a change to Twister know that their change is not breaking for Windows? To verify that, we ought to have a working CI which built and runs Twister tests for Windows and does a check on PRs which change Twister in any way. We do not have such a workflow here, but I've conducted a few test workflow run in a private repo. All unit tests pass on both Windows and Linux after those changes. Without them, unit tests fail on Windows. With Unit Tests failing on Windows, we cannot have a Windows-specific CI runs and thus cannot in any way safeguard future Windows correctness. This is not a singular PR, but a step on the way towards better Windows support. |
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
from
fd746d8 to
82d8ae4
Compare
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
from
82d8ae4 to
7d5261a
Compare
Although we do not run it for every PR, there is multi-platform CI workflow using Twister here: We also run Windows CI using Twister in the SDK repository, which works without this change: So your claim about unit tests failing on Windows without this change is invalid. p.s. I am not saying these changes are bad or unnecessary -- I am just pointing out that the status quo is Twister on Windows works without any of these changes, unlike your claim that Twister on Windows is currently broken and needs to be fixed. |
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
6 times, most recently
from
f634c10 to
ccf3779
Compare
|
Presence of semi-regular Windows errors, like #82020 indicate need for Blackbox testing on Windows in our CI. I'll investigate whether change in repo allow for this without major code changes. |
henrikbrixandersen
added
the
area: Windows Support
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
from
ccf3779 to
3243ab3
Compare
Unit tests were failing on Windows, indicating that current Twister code is not truly multiplatform. Linux-specific code parts were changed into multiplatform ones. Paths were deemed the main culprit, so this commit introduces a new TPath, that aims to fix the Windows limitation of supporting only up to 260 char paths by default. Signed-off-by: Lukasz Mrugala <[email protected]>
LukaszMrugala
force-pushed
the
lmrugalx_twister_ut_windows
branch
from
3243ab3 to
9c02226
Compare
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
8 participants
Unit tests were failing on Windows, indicating that current Twister code is not truly multiplatform.
Problems detected:
TPath, that aims to fix the Windows limitation of supporting only up to 260 char paths by default via a path prefix disabling this limitation.TestSuitenameformation was updated, so it wouldn't create extremely long path elements as its name, given thatTestSuitenameis used in dir creation.os.killwith a Linux-specific signal is used.Processis used instead.pathlib'sWindowPaths were not serialised correctly in the reports.PosixPathserialising fix was extended to other path classes.Unit tests were adjusted (and some disabled, as they didn't make sense on a given OS).