Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VisualStudioReporter incorrect comparison order of approved and received? #549

Closed
MihailsKuzmins opened this issue Aug 3, 2021 · 6 comments

Comments

@MihailsKuzmins
Copy link

When files are compared by the VisualStudioReporter "approved.json" is displayed on the right-hand side and "reveived.json" is displayed on the left-hand side. For it this order is a very misleading because in git a new file (i.e. "reveicied.json") is shown on the right-hand side.
So the current implementation of ApprovalTests makes me think that "approved.json" is the latest received file. Please consider this image.
When I look at this image I think that the latest file has a new field "Version". HOWEVER, actually the situation is the opposite. The field "Version" has been removed in the latest file. That's why I consider this implementation very confusing.
image

Apparently this method needs to be overridden in VisualStudioReporter.cs. If you agree that it could be a good improvement I could also try to prepare a PR for it.

public void Report(string approved, string received)
{
DiffRunner.Launch(diffTool, received, approved);
}

@MihailsKuzmins MihailsKuzmins changed the title VisualStudioReporter incorrect comparison of approved and received? VisualStudioReporter incorrect comparison order of approved and received? Aug 3, 2021
@SimonCropp
Copy link
Contributor

@MihailsKuzmins

changing DiffRunner.Launch(diffTool, received, approved); wont work DiffRunner.Launch has logic in it that assumes the received and approved are passed in in the correct order.

If you want to change this, it should be a PR in https://github.com/VerifyTests/DiffEngine that inverts the display order via an environment setting

@MihailsKuzmins
Copy link
Author

MihailsKuzmins commented Aug 4, 2021

@SimonCropp, thank you that you corrected my mistake of the design.
So as I understood VisualStudio.cs must be changed, to be more specific this line (temp, target) => $"/diff \"{temp}\" \"{target}\"",. So the first file is "received" and the second is "approved".

public void Report(string approved, string received)
{
DiffRunner.Launch(diffTool, received, approved);
}

So I ran this test. And indeed the "received.json" was on the left-hand side.
So in VisualStudio.cs I only made this change and after that the result was as I would expect ("received.json" was on the right-hand side)
image
image

If you agree that this change is all right I will prepare a PR in the DiffEngine.

@SimonCropp
Copy link
Contributor

discussing on diffengine repo so will close this

@SimonCropp
Copy link
Contributor

this can now be controlled using an environment variable https://github.com/VerifyTests/DiffEngine/blob/main/docs/diff-tool.md#leftright-diff-behavior

@MihailsKuzmins
Copy link
Author

@SimonCropp,
thank you so much, your solution is more general and will cover similar cases in other reporters.
Tested the new version and it works as expected. Waiting for the new NuGet release

Environment.SetEnvironmentVariable("DiffEngine_TargetOnLeft", "true");

@SimonCropp
Copy link
Contributor

the nuget is deployed. but note that you should not add SetEnvironmentVariable("DiffEngine_TargetOnLeft to your code

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

No branches or pull requests

2 participants