Allow layers control section to resize to contents #5474
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5474 +/- ##
==========================================
- Coverage 89.30% 89.29% -0.01%
==========================================
Files 600 600
Lines 51030 51039 +9
==========================================
+ Hits 45570 45576 +6
- Misses 5460 5463 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
|
Not sure what's up with Windows CI, but I've re-run it twice and it's the same, so I think it's not random? |
There was a problem hiding this comment.
Awesome! This seems like the "correct" solution :)
Not sure what's up with Windows CI, but I've re-run it twice and it's the same, so I think it's not random?
This looks familiar... I was stuck due to a bunch of these failures in #5312, and I had to ultimately disable some tests on windows CI. Not sure if it's the same here...
|
I tried bumping windows CI to python 3.9 (maybe we should do that anyways, since that's the version we suggest in installation?) but it didn't help. Can we assume from the 3 dots after skip: That the last 3 tests completed, so it's the first of the next test file? |
|
could you try to run a verbose test (to find failing job). It will be also nice to try to run only |
|
I got pyqt to pass with +1 skip Edit: also I was not reading the Edit: it's like whack-a-mole, the viewer tests pass (with skips) but now octree fails: |
|
Ok, I ended up having to skip 3 tests plus the octree. The octree one fails and gives a similar error from the subprocess, so I think it's related, but 🤷♂️ |
|
Should I try checking if reverting the min-height change in the qss file helps with the failing checks? |
I guess? hard to imagine that being the culprit, but your commit before mu suggestion seems to have flown by. However, my fork branch is using the 295 value and still shows the same CI issues. |
|
We also have some flaky tests that w're slowly gettin grid of, so some failures are unfortunately fake :/ |
dalthviz
force-pushed
the
issue_2337_5472
branch
from
b20243e
to
0ddd2d5
Compare
dalthviz
force-pushed
the
issue_2337_5472
branch
from
fd8448a
to
0ddd2d5
Compare
dalthviz
force-pushed
the
issue_2337_5472
branch
from
8d44a64
to
8684b71
Compare
dalthviz
force-pushed
the
issue_2337_5472
branch
from
8684b71
to
a1353bb
Compare
dalthviz
force-pushed
the
issue_2337_5472
branch
from
a1353bb
to
6010cff
Compare
|
Note: seems like the skipped tests are failing due to an access violation error on Windows when Vispy code is creating/managing the OpenGL related context/objects? Some tracebacks/info from the errors/failures on skipped tests:
Checked trying to reproduce the test suite failing locally on Windows setting Also, checking the CI and local test suite final message seems like an error message from
|
|
Note: Re-runs of the bundle workflow jobs for failures to upload artifacts probably due to GitHub Actions degradation status. See actions/upload-artifact#383 and https://www.githubstatus.com/incidents/n18j5g7wpg0s |
Yeah those failures are familiar... Very annoying, sorry about that :/ |
|
Before merging, how do others feel about these skipped tests? Should we be more responsible and try to solve the problem instead of shoving it under the rug, or are we happy like this? @Czaki @andy-sweet tagging you since the three of us particularly have fought with these flaky tests before... |
|
I do not understand how these changes may reuires so many changes in tests. |
My assumption is that the skipped tests have underlying issues that are independent of the implementation changes in this PR that mean they fail sometimes. The implementation changes here affect the layout, which in turn affects drawing the widgets, which is something that could be relatively slow on CI runners. My other more speculative guess is that the Windows runners might be a bit slower in this regard and that's the reason we often skip on Windows CI only rather than there being an actual Windows issue. We already have quite a lot of skips, so I have no issue with the ones added here, especially given the fix is simple and desirable. |
|
Ok, I guess I'll merge then. Let's hope that as these issues keep showing up, we can figure out exactly how to prevent them :) |
Czaki
added
the
triaged-0.4.18
Description
Set
min-heightandmin-widthto the empty widget used by the layers controls container widget instead of the widget itself to allow it to resize dependending on the controls being shownPreview
Type of change
References
closes #2337
closes #5472
How has this been tested?
as there are small differences between the two Qt bindings.
Final checklist: