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

move to verify #1165

Merged
merged 7 commits into from
Mar 29, 2020
Merged

move to verify #1165

merged 7 commits into from
Mar 29, 2020

Conversation

SimonCropp
Copy link
Contributor

@clairernovotny this approach does not use pdbs to determine the path. can u verify it work for your deterministic builds?

if it does, and u are happy with this as an approach, i will finish the diffplex logging and u can merge

@clairernovotny
Copy link
Member

The approach seems fine to me, thank you! There's a unit test failing though because the .cs files are missing a license header.

@SimonCropp
Copy link
Contributor Author

SimonCropp commented Mar 28, 2020

yep fixed the license header

@@ -53,7 +53,7 @@ private int ScanPath(string path, StringBuilder error)
|| file.Contains("AssemblyInfo.cs")
|| file.Contains(".Designer.cs")
|| file.Contains(".Generated.cs")
|| file.Contains(".approved.cs")
|| file.Contains(".verified.cs")
Copy link
Member

Choose a reason for hiding this comment

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

Cool, LGTM!

@SimonCropp
Copy link
Contributor Author

ok. i will add a extension point to Verify so u can intercept the files on failure.

@SimonCropp
Copy link
Contributor Author

unfortunately in the end i had to add the following to the csproj to get this to work

<ContinuousIntegrationBuild>false</ContinuousIntegrationBuild>
<Deterministic>false</Deterministic>

The reason being is that Deterministic seems to not just clean pdb paths (which breaks approvaltests), it also cleans CallerFilePath attributes (which breaks Verify).

So with that workaround in mind, it is up to you if you want to use Verify or ApprovalTests

@SimonCropp SimonCropp changed the title WIP: move to verify move to verify Mar 29, 2020
@clairernovotny
Copy link
Member

What's the difference between ApprovalTests or Verify?

@SimonCropp
Copy link
Contributor Author

@clairernovotny
Copy link
Member

Cool, I'll stick with Verify. For now, I can set Deterministic to false in the Approvals project. Might be useful to think about an alternative way to get the directory needed though, perhaps with build-time env variables. You can detect deterministic paths by looking for /_/ as the root and take action accordingly.

@clairernovotny clairernovotny merged commit 9cb5907 into dotnet:master Mar 29, 2020
@SimonCropp SimonCropp deleted the moveToVerify branch March 29, 2020 21:04
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.

2 participants