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

Resolve build errors in test project #577

Merged
merged 6 commits into from
Sep 8, 2022
Merged

Conversation

steverichey
Copy link
Contributor

@steverichey steverichey commented Sep 6, 2022

Fixes #504.

There were two issues causing build errors in the test project:

  • A reference to DiffEngine 8.5.3, while an indirect reference through another dependency required 9.1.0
  • Use of the now removed VerifierSettings.ModifySerialization

By upgrading the DiffEngine reference and updating the methods used to set up Verify, I am now able to build this project and run tests locally. However, some tests still fail.

I've also updated dependencies in the ExampleClient project to allow the auto-format action to succeed.

using DiffEngine 8.5.3 was causing package downgrade errors
ModifySerialization removed: VerifyTests/Verify#533
DontScrubNumericIds removed: VerifyTests/Verify#526
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@steverichey
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

this resolves a package downgrade error
@steverichey
Copy link
Contributor Author

If desired, I can submit a subsequent pull request to resolve test failures.

@mscottford
Copy link
Member

Thanks, @steverichey! Instead of a separate pull request, it's best to fix the test failures as part of this one.

@steverichey
Copy link
Contributor Author

Well, this is confusing. Reverting to a version of the ElasticSearch client before they added pre-flight checks resolves the issues for many tests. However, the solution to the second test issue is less clear. Here's an example test failure:

Corgibytes.Freshli.Lib.Test.Integration.Languages.Ruby.RubyGemsRepositoryProperlyHandlesRateLimitsTest.FullSet
Source: RubyGemsRepositoryProperlyHandlesRateLimitsTest.cs line 24
Duration: 1.1 min
Message: 
System.MissingMethodException : Method not found: 'System.ValueTuple`2<System.String,System.String> ReflectionFileNameBuilder.FileNamePrefix(System.Reflection.MethodInfo, System.Type, System.String, VerifyTests.VerifySettings, System.String)'.

Stack Trace: 
<>c__DisplayClass0_0.b__0(String uniqueness)
InnerVerifier.ctor(String sourceFile, VerifySettings settings, GetFileConvention fileConvention) line 17
Verifier.GetVerifier(VerifySettings settings, String sourceFile) line 14
<b__0>d.MoveNext() line 31
--- End of stack trace from previous location ---

At the end of the FullSet test, we call Verifier.Verify(packagesAndResults). Under the hood, this works its way to this method which in turn expects a method ReflectionFileNameBuilder.FileNamePrefix. This method is in the Verify assembly and I was able to load it at runtime by adding these lines to the test:

var assembly = AppDomain.CurrentDomain.GetAssemblies().Single(assembly => assembly.GetName().Name == "Verify");
var rfnb = assembly.GetType("ReflectionFileNameBuilder", true);
var methd = rfnb.GetMethod("FileNamePrefix", BindingFlags.Static | BindingFlags.Public);

It's not immediately clear why this method can't be loaded. Will investigate as time allows.

@mscottford
Copy link
Member

There might be a breaking change in the Verify package that we need to contend with. Perhaps searching for an older version of it?

this is a sort of goldilocks zone for this dependency: newer versions change the api, and older versions have an unavoidable missingmethodexception error. unfortunately, some change to the library causes 2.0.0 and 2.0 to swap places in verifier output, resulting in a necessary change to a .verified.txt file. i think this is a reasonable tradeoff.
@steverichey
Copy link
Contributor Author

steverichey commented Sep 7, 2022

I'm still not sure what was causing the MissingMethodException, but it continues to be an issue until 16.2.0. As of 16.3.0, the issue goes away. This change to the .csproj file seems a likely culprit, as missing target frameworks is the most common cause of methods not being included in an assembly, in my experience. The pull request that made this change is vague, but seems designed to improve .net6 support, which we are currently using. I've updated the tests project to use 16.3.0, but this does change how some verification results are sorted, swapping 2.0.0 and 2.0, which you can see here.

@steverichey
Copy link
Contributor Author

steverichey commented Sep 7, 2022

@mscottford Take a look at the changes when you have a chance. The most recent macOS tests failed, but it seems due to a spurious connection failure. The solution for this -- in the very long term -- would be to stub or mock external connections to avoid brittle tests, IMHO.

@mscottford
Copy link
Member

We’re going to deprecate this library soon (a month or so). Let’s go ahead and remove the test matrix so we’re only testing against Linux. That will get this project passing well enough for us to move forward.

Copy link
Member

@mscottford mscottford left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! This looks good enough to merge after the test matrix is changed to only run Linux builds.

@@ -10,17 +10,13 @@ public static void Initialize()
{
VerifyDiffPlex.Initialize();

VerifierSettings.ModifySerialization(settings =>
{
settings.DontScrubNumericIds();
Copy link
Member

Choose a reason for hiding this comment

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

Is there no equivalent for this call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I'm aware, it was completely removed. See: VerifyTests/Verify#526

@steverichey steverichey merged commit 9493e48 into main Sep 8, 2022
@steverichey steverichey deleted the feature/build-errors branch September 8, 2022 13:54
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2022
@mscottford mscottford added the bug Something isn't working label Sep 8, 2022
@mscottford mscottford added this to the v0.5.0 milestone Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project won't build
2 participants