Closed egil closed 1 year ago
RenderedFragmentConverter does not seem to be used?
Used here https://github.com/VerifyTests/Verify.Blazor/blob/main/src/Verify.Bunit/VerifyBunit.cs#L28
It seems redundant to convert fragment.Nodes to markup just to convert them back to nodes again in the comparer. But as far as I can tell there is no way around this with Verify, right?
unfortunately no. since comparers are designed to work on the rendered result. but perf wise it is usually not an issue since the comparer on files if the text diff resolves to notequal
As mentioned, this utilizes the differ built-in to bUnit. That has the advantage of avoiding tests breaking if bUnit internally makes changes, and it handles some of the special attributes bUnit adds to the output.
Yep i like this approach
This defaults to using the semantic comparer (based on Anglesharp.Diffing) that bUnit comes with, which has been configured to ignore bUnits attribute in the markup, and it includes a custom node matcher that is needed in some circumstances.
I like it. was not aware buint had a comparer
Ideally, users should not be asserting against the internal state of a component. Even though the component is a public thing, accessing its parameters and other public properties is not considered good practice in frontend testing, since you are interacting and accessing the component in a way that the end user will not. The end user will look at the markup only. Thus, by default, I suggest only verifying the produced markup.
Not sure i agree with that. component state can be helpful in a snapshot. the rendered result can somewhat obscure the state, and also sometime omit some useful info. But i am happy to have a option to exclude component state .
I did attempt to set BunitFileExtension to bunit, such that the compare would only be used with it, avoiding any conflicts with non-IRenderedFragment compares that may be registered. But it does not seem to be possible.
yeah that would be difficult. Verify is aware of "what extensions are text" and treats them differently. but it should be possible. I will take a look at it
I am not super familiar with Verify so I wanted to dump my suggestion here instead of opening a PR.
Based on the above i think a few smaller PRs would work better. so we can discuss the specifics of each individual change
Alternatively. I can do a PR to the bunit codebase that moves this project into yours, and make u the owner of the Verify.BUnit nuget. Obviously still happy to help u out, but it would mean you can take Verify.BUnit any direction u want.
For context when i created Verify.BUnit, i was primarily working in blazor. Now i am back on full time backend work, so not doing any blazor.
RenderedFragmentConverter does not seem to be used?
Used here https://github.com/VerifyTests/Verify.Blazor/blob/main/src/Verify.Bunit/VerifyBunit.cs#L28
Yes, I can see it is being referenced. I just haven't found a way to use verify yet that causes the type to be used (if I set a breakpoint in the RenderedFragmentConverter.Write
method). So I am not sure when it would be used.
It seems redundant to convert fragment.Nodes to markup just to convert them back to nodes again in the comparer. But as far as I can tell there is no way around this with Verify, right?
unfortunately no. since comparers are designed to work on the rendered result. but perf wise it is usually not an issue since the comparer on files if the text diff resolves to notequal
Yeah, I get that. It makes sense. The verified content is in text form, so it makes sense that all comparisons are done based on text.
As mentioned, this utilizes the differ built-in to bUnit. That has the advantage of avoiding tests breaking if bUnit internally makes changes, and it handles some of the special attributes bUnit adds to the output.
Yep i like this approach
This defaults to using the semantic comparer (based on Anglesharp.Diffing) that bUnit comes with, which has been configured to ignore bUnits attribute in the markup, and it includes a custom node matcher that is needed in some circumstances.
I like it. was not aware buint had a comparer
Ideally, users should not be asserting against the internal state of a component. Even though the component is a public thing, accessing its parameters and other public properties is not considered good practice in frontend testing, since you are interacting and accessing the component in a way that the end user will not. The end user will look at the markup only. Thus, by default, I suggest only verifying the produced markup.
Not sure i agree with that. component state can be helpful in a snapshot. the rendered result can somewhat obscure the state, and also sometime omit some useful info. But i am happy to have a option to exclude component state .
It is not a hard rule, we do expose the component instance since there are legitimate cases where you have to do that. But in many cases, the components state is an implementation detail, which tests generally should not depend on. The externally observable behavior of a component, the rendered output, or the external collaborators a component interacts with, is what should be asserted against. Not a hard rule, but a good property to aim for when testing.
I did attempt to set BunitFileExtension to bunit, such that the compare would only be used with it, avoiding any conflicts with non-IRenderedFragment compares that may be registered. But it does not seem to be possible.
yeah that would be difficult. Verify is aware of "what extensions are text" and treats them differently. but it should be possible. I will take a look at it
This is a double edge sword. It would be nice to avoid having the rendered fragment
specific compare used for other html
based content, but it also makes sense that the file extension on the verified/received files is html
. So I guess I was looking for a way to ensure that the comparer here was only used to compare HTML produced by a bUnit IRenderedFragment
.
I am not super familiar with Verify so I wanted to dump my suggestion here instead of opening a PR.
Based on the above i think a few smaller PRs would work better. so we can discuss the specifics of each individual change
How would you like to break this up?
Besides the CreateDiffMessage
, I think for the most part, all of it goes together. I am not certain if there needs to be changes to the RenderedFragmentConverter
since I have not seen it being used though.
Alternatively. I can do a PR to the bunit codebase that moves this project into yours, and make u the owner of the Verify.BUnit nuget. Obviously still happy to help u out, but it would mean you can take Verify.BUnit any direction u want.
I think it makes sense that this stays with Verify, as it has the package prefix and heavily depends on Verify as a framework. There is a certain style and expectations related to how a verify project works that verify users probably have come to expect, and I probably don't have the bandwidth to keep up with that.
For context when i created Verify.BUnit, i was primarily working in blazor. Now i am back on full time backend work, so not doing any blazor.
I am also mostly a backender at work too :)
Hi @SimonCropp et al,
I have been experimenting with Verify.Bunit and have some ideas for improving it, such that it leverages the pretty printer and differ built into bUnit. This ensures that bUnit's
blazor:xxx
attributes are removed and ignored, and a diff is performed using AngleSharp.Diffing with the same configuration that bUnits "inline snapshot testing" methodMarkupMatchces
uses.My attempt at a
VerifyBunit
type looks like this, with lots of copy paste from the current implementation:The above code compiles with the following package references:
A few questions/comments:
RenderedFragmentConverter
does not seem to be used?fragment.Nodes
to markup just to convert them back to nodes again in the comparer. But as far as I can tell there is no way around this with Verify, right?BunitFileExtension
tobunit
, such that the compare would only be used with it, avoiding any conflicts with non-IRenderedFragment
compares that may be registered. But it does not seem to be possible.I am not super familiar with Verify so I wanted to dump my suggestion here instead of opening a PR.