search

bloomberg / memray

Memray is a memory profiler for Python
https://bloomberg.github.io/memray/
Apache License 2.0
13.17k stars 392 forks source link

Update the textual snapshots #616

Closed pablogsal closed 4 months ago

pablogsal commented 4 months ago

Issue number of the reported bug or feature request: #

Describe your changes A clear and concise description of the changes you have made.

Testing performed Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.

Additional context Add any other context about your contribution here.

pablogsal commented 4 months ago

Hey @willmcgugan! 👋

Unfortunately this is the 3rd or 4th time this week we are broken by changes in textual and we need to either apply fixes or refresh the snapshots. Unfortunately this is becoming a bit of a problems as is raising our maintenance burden quite a lot. Is there any way we can use the snapshot plugin to not be constantly broken on every new release of textual?

Thanks a lot for the help!

willmcgugan commented 4 months ago

Hi Pablo. Sorry about that. The only way to avoid snapshots breakages would be to pin Textual. Then you could bump it at your leisure. I'd recommend that anyway, since we're still in zerover.

pablogsal commented 4 months ago

Hi Pablo. Sorry about that. The only way to avoid snapshots breakages would be to pin Textual. Then you could bump it at your leisure. I'd recommend that anyway, since we're still in zerover.

We have considered this but the problem is that this prevents most distributions to package textual and memray together as they try to get textual as updated as possible.

pablogsal commented 4 months ago

In any case if there isn't a better solution that's fine, we can try to discuss about what's the best balance we want between the memray maintainers :)

willmcgugan commented 4 months ago

If its any consolation, the recent breakages were related to an update to the footer. There likely aren't going to be many such snapshot breaking updates for a while.

pablogsal commented 4 months ago

If its any consolation, the recent breakages were related to an update to the footer. There likely aren't going to be many such snapshot breaking updates for a while.

Yup, we fixed those here: https://github.com/bloomberg/memray/pull/613

godlygeek commented 4 months ago

The breakage being fixed by this PR isn't from the footer, actually. It seems to be two changes, both introduced in v0.63.6:

  1. A horizontal scrollbar got slightly wider:

image

  1. One of the columns in our data table is now one cell less wide than before:

image

(See that the "%" of the "% Own" column used to be under the "2" at the bottom of the "Heap Usage" box and is now under the space before the "2")

These might both be Textual regressions, actually, since the changelog for v0.63.6 doesn't mention either of them, and the snapshots pass with v0.63.5...

godlygeek commented 4 months ago

git bisect says that it is https://github.com/Textualize/textual/commit/b3582f97dbd32022a0d4828ef5698f8f1c328fbe which changed both of those things, but the commit message doesn't suggest that changing either of those things was intentional...

willmcgugan commented 4 months ago

I suspect the broken snapshots are only tangentially related to the commit. We had issues with our snapshot tests, where they would fail intermittently or with unrelated changes. When we looked in to them, they all turned out to be legitimate race conditions that we could fix. Textual being an async framework is quite sensitive to timing. You may find that your test code isn't running as predictably as you might expect.

Can't say conclusively that the commit isn't to blame, but we have well over 100 tests and we didn't notice any scrollbar or datatable changes...

godlygeek commented 4 months ago

These two changes are definitely one change, actually - the data table is getting one column narrower (because one column is shrinking), and that's why the scrollbar now shows as slightly longer in the one snapshot (the visible columns represent a larger proportion of the total columns, since there's one less column in total).

So the only mystery is why the column is shrinking.

It could be that that's always been shrinking on refresh and our snapshots were previously being taken before that refresh, and are now being taken after that refresh, though I would have expected snap_compare to be taking care of that automatically, since it's supposed to wait until the app is idle and there aren't pending messages before it takes the snapshot, IIRC...

godlygeek commented 4 months ago

It's https://github.com/bloomberg/memray/blob/0e3673d7fd032577c8a60396132397cee44b90be/tests/unit/test_tui_reporter.py#L712 that's failing:

FAILED tests/unit/test_tui_reporter.py::test_tui_basic[narrow-terminal-short-snapshots] - assert False
FAILED tests/unit/test_tui_reporter.py::test_tui_basic[wide-terminal-long-snapshots] - assert False
FAILED tests/unit/test_tui_reporter.py::test_tui_basic[very-wide-terminal-short-snapshots] - assert False

If there's anything we're doing wrong there, it'd have to be in how we're driving the pilot in the compare fixture.

godlygeek commented 4 months ago

Well, while investigating this, I noticed a real bug that we've somehow never noticed: we're incorrectly computing the "Own Memory" column. #617 fixes that bug, and since it also needs to update the snapshots, we don't need to land this PR and can just land that one.

willmcgugan commented 4 months ago

There may be an issue with the way you take snapshots in run_before. That code runs concurrently when the app starts, when not all messages have been processed. The screenshot it generates may not be the first "frame" the user sees. It would also make it sensitive to any change of order of messages in Textual. i.e. it could break due to change to Textual that we have confirmed are inconsequential for the user. If you have been seeing a lot of snapshot fails not related to any widget we have updated, that might be the explanation.

It may make it more reliable to await at least a pilot.pause() before taking a snapshot, but TBH run_before wasn't intended to be a place to take snapshots. It was more of a place to simulate user events. Here's the snapshot tests we run in Textual.