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

Update tests' target framework #140

Merged
merged 6 commits into from
Aug 3, 2018
Merged

Update tests' target framework #140

merged 6 commits into from
Aug 3, 2018

Conversation

martincostello
Copy link
Contributor

Update the tests to target netcoreapp2.1 as well as update the .NET Core test SDK and xunit NuGet package references.

The xunit upgrade creates some code analysis warnings as xunit now ships with code analyzers, so they have also been fixed.

Originally part of #131.

Update the unit tests to target netcoreapp2.1.
Update to the latest .NET Core test SDK.
Update to the latest version of xunit.
Fix code analyzer warnings generated by upgrade to xunit.
Update the Target Framework Moniker for the integration tests to 2.1 and update the test SDK as well, as they should have been done as part of 2a1d6c7 but got missed.
@martincostello martincostello mentioned this pull request Jul 31, 2018
2 tasks
@martincostello
Copy link
Contributor Author

As an unintended side-effect of the runtime upgrade, some of the tests in ExpressionRewriterTests are now failing because the ToString() representations now include the airity of the type (so IEnumerable`1 instead of IEnumerable).

I wasn't sure the best way to fix these, so for now they're still broken. Will fix once there's a recommended approach.

@StanleyGoldman
Copy link
Collaborator

You can go ahead and change the strings in the ExpressionRewriterTests, they are there to prove we know what the expression being built is and to protect us from any unintended changes. Our asssumptions are still correct, it's just the string representation of them became more specific.

I'm going to let @grokys give you the final approval on this pull request as he is more savvy about .net core changes than I am. It is probably fine, but I'll let him make the call.

@martincostello
Copy link
Contributor Author

Hmm - on closer inspection, I don't think that's going to be particularly maintainable.

For example, this is the actual string for one of the expressions now (from Licenses_Conditions_Select_ToDictionary), and with it also containing the names of anonymous types, they could be quite brittle to compiler-refactoring in the future.

"data => Convert(Select(data.get_Item(\"data\").get_Item(\"licenses\"), x => new <>f__AnonymousType2`2(Body = x.get_Item(\"body\").ToObject(), Items = Convert(Select(x.get_Item(\"items\"), i => new <>f__AnonymousType3`2(Key = i.get_Item(\"key\").ToObject(), Description = i.get_Item(\"description\").ToObject())).ToDictionary(d => d.Key, d => d.Description), IDictionary`2))).ToList(), IEnumerable`1)"

@martincostello
Copy link
Contributor Author

So far all I can think of is asserting against the literal expected to be produced by the compiled query like this:

        [Fact]
        public void Licence_Conditions_Nested_Selects()
        {
            var expression = new Query()
                .Licenses
                .Select(x => new
                {
                    x.Body,
                    Items = x.Conditions.Select(i => i.Description).ToList(),
                });

            var actualQuery = expression.Compile().ToString();
            var expectedQuery = @"
query {
  licenses {
    body
    items: conditions {
      description
    }
  }
}
";

            Assert.Equal(expectedQuery.Trim(), actualQuery);
        }

@StanleyGoldman
Copy link
Collaborator

Sorry, I was not paying enough attention to what you were saying. These tests are tricky, I pushed the fix to them.

@StanleyGoldman
Copy link
Collaborator

So to explain the tests I just fixed, when we process a query we have to generate two things.

  1. The actual graphql string we need to send to the connection
  2. An expression that will process the JObject/JArray we get as a result of the queries execution

The ExpressionRewriterTests are showing that expression we generate to process the resultant data is correct.

All that said, just like you mentioned, there are tests that test the query we generate. Like this one...

[Fact]
public void RepositoryOwner_Repository_Query()
{
var expected = @"query {
repositoryOwner(login: ""foo"") {
repository(name: ""bar"") {
id
name
owner {
login
}
isFork
isPrivate
}
}
}";
var expression = new Query()
.RepositoryOwner("foo")
.Repository("bar")
.Select(x => new
{
x.Id,
x.Name,
Owner = x.Owner.Select(o => new
{
o.Login
}).Single(),
x.IsFork,
x.IsPrivate,
});
var query = expression.Compile();
Assert.Equal(expected, query.ToString(), ignoreLineEndingDifferences: true);
}

@martincostello
Copy link
Contributor Author

Ah, that makes now.

I’d circled slightly around near that and almost stumbled across the fix myself, as VS was warning my about a redundant IEnumerable cast. I went the other way and just removed it, but that didn’t fix it.

@StanleyGoldman
Copy link
Collaborator

Yep, it would definitely be redundant for someone to actually write that in code.
Some of our tests are tricky, you'll get the hang of them.

Remove NuGet package references that snuck in from #131.
@martincostello
Copy link
Contributor Author

Some references snuck in when I extracted this from #131 - fixed now.

@StanleyGoldman
Copy link
Collaborator

Good catch.

@grokys
Copy link
Collaborator

grokys commented Aug 2, 2018

Hi @martincostello - what was the reasoning behind updating the unit tests' framework?

@martincostello
Copy link
Contributor Author

@grokys Two reasons:

  1. I need 2.1 for HttpClient injection #131 to test usage with IHttpClientFactory.
  2. I don't even have 1.1 installed on my laptop (because it's an old LTS release now), so I couldn't run the tests from Visual Studio and didn't really want to have to install it just to use an old version of the runtime.

@grokys
Copy link
Collaborator

grokys commented Aug 3, 2018

@martincostello ok, thanks for explaining - they sound like good enough reasons to me!

# Conflicts:
#	Octokit.GraphQL.IntegrationTests/Queries/IssueTests.cs
Copy link
Collaborator

@grokys grokys left a comment

Choose a reason for hiding this comment

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

Thanks a lot @martincostello - looks good to me.

@StanleyGoldman StanleyGoldman merged commit 3e85171 into octokit:master Aug 3, 2018
@martincostello martincostello deleted the Update-Test-Target-Framework branch August 3, 2018 14:48
@StanleyGoldman StanleyGoldman added this to the v0.1.1-beta milestone Aug 10, 2018
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