Skip to content

Testing: profiles for padding, padding color, post-creation config#1462

Open
DJMcNab wants to merge 5 commits intolinebender:mainfrom
DJMcNab:padding-color
Open

Testing: profiles for padding, padding color, post-creation config#1462
DJMcNab wants to merge 5 commits intolinebender:mainfrom
DJMcNab:padding-color

Conversation

@DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Nov 7, 2025

Adds support for configuring how padding in renders done by Masonry Testing is done at runtime.
Also adds support for configuring the padding colour.

Additionally, this adds targeted methods for the archetypes of screenshots identified in #1457

This is now applied to button (and text_input, as if it were the default)

…tion

Adds support for configuring how padding in renders done by Masonry Testing is done at runtime.
Also adds support for configuring the padding colour

Additionally, this adds targeted methods for the
archetypes of screenshots identified in
linebender#1457
Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

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

I don't remember if we talked about this, so I'll just leave a review now: I'm not a fan of the new padding color in the changed screenshots. In fact, I'd maybe consider it a regression.

I think having the padding color be a different color than the background (e.g. what you see behind the rounded corners) is visually confusing and kind of defeats the point of adding padding in the first place.

I'm open to changing my mind if there's a strong rationale though.

@DJMcNab
Copy link
Member Author

DJMcNab commented Nov 21, 2025

I'm happy to remove the colour. I still think that having padding in the vast majority of screenshot tests is valuable:

  • It makes intentionally "overdrawn" content visible (e.g. shadows, focus indicators)
  • It makes screenshots easier to parse in that the widgets are shown more "in-situ"

I do feel that having some indication of what the padding region is does have value, in that it allows "unintentional" overdraw to be caught. Overdraw can at least be manually detected with the debug paint?


As a procedural note, we seem to be talking past each other. Once we remove the default padding colour from this PR (or in the follow-up to actually enable the default padding), we're back in the state from before #1457 (review), in that the newly added default padding wouldn't be discoverable.

However, there is another assumption which this PR is making, which might not be true, which is that the "screenshot of individual widgets" case is the common-case. It certainly is in this repo, but it really isn't for users of Masonry. Because of this, it feels like there might be some solution which satisfies all of our needs.
I still strongly feel that making every test opt-in to padding is a really poor solution, for the reasons I've already outlined (in the happy case it's just boilerplate, and for tests which accidentally don't opt-in, it makes for confusing inconsistencies).
However, if it is the case that for a given TestHarness, it's serving one of the identified roles in about equal measure, suddenly a more explicit API becomes more appealing. However, I feel that in that case, setting which class of test harness you explicitly are needs to be enforced, i.e. it's not like you say TestHarness::new makes a test harness with no padding, which isn't actually what we recommend for anyone to use.

I'm not sure however how to make such an explicit API ergonomic, whilst still being explicit enough that padding is being added to meet your discover-ability targets (e.g. saying TestPadding::SingleWidget adds a lot of characters, and will push rustfmt into a very verbose formatting).

@xStrom
Copy link
Member

xStrom commented Jan 10, 2026

I think that having some amount of padding for every test screenshot has value, because yes, otherwise things like box shadow will not be tested in some cases at all. Also, I think there should be a sane default value and so only test cases that really need to will override it.

I'm less convinced that it should be a different color than the rest of the background. It can perhaps be of value in the single widget screenshot case, but we have a lot of multi-widget tests where it makes less sense. Looking at the current screenshot set, most of them have multiple different widgets in them. Also, all the new tests I'm currently planning of adding are going to be multi-widget.

In any case, I think both the background and padding should be of a dark color and the actual interesting small pixels of a light color. That is because the human eye is more sensetive to light, so you want the small pixels to be bright. This current light blue padding color goes against that. Having a box shadow that is dark also goes against it, and so our testing theme should have a light colored shadow. Basically, I think the testing theme should be a very high contrast dark theme.

@PoignardAzur
Copy link
Contributor

PoignardAzur commented Jan 10, 2026

After having had time to think on it, I'm strongly in favor of:

  • Actually use a padding widget, even if that's slightly more code.
  • Don't have "silent" padding by default, but have multiple TestHarness constructors with a padding argument.

I still think having padding without the user explicitly specifying it is bad and confusing. Padding in general is "insidious", it's easy to add it and forget about it, if you have multiple sources of it then it's hard to tell them apart, and if you don't know it's there it's not discoverable.

I don't think making the padding a different color than the background is a good workaround: overall test padding should be used to simulate the widget being in a "bigger" window; it shouldn't be a decorated border.

Basically, I think the testing theme should be a very high contrast dark theme.

That makes sense, yeah.

@xStrom
Copy link
Member

xStrom commented Jan 10, 2026

  • Actually use a padding widget, even if that's slightly more code.

I'm not a fan of styling widgets at all. I think padding on widgets should be a property. Having a whole series of widgets to do styling seems like a performance cliff.

  • Don't have "silent" padding by default, but have multiple TestHarness constructors with a padding argument.

I mean, I guess it's not too bad to have a , 20.px() added to every constructor call, so I could live with that.

@PoignardAzur
Copy link
Contributor

PoignardAzur commented Jan 10, 2026

I'm not a fan of styling widgets at all. I think padding on widgets should be a property. Having a whole series of widgets to do styling seems like a performance cliff.

That one would be an implementation detail of masonry_testing, not an exported widget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants