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

migrate odata client tests to xUnit #1759

Conversation

ElizabethOkerio
Copy link
Contributor

Issues

This pull request fixes issue #xxx.

Description

Briefly describe the changes of this pull request.

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

@ElizabethOkerio ElizabethOkerio force-pushed the fix/migrate_odata_client_tests_to_xunit branch from 800d9b6 to e95fef9 Compare May 18, 2020 18:53
@ElizabethOkerio ElizabethOkerio force-pushed the fix/migrate_odata_client_tests_to_xunit branch from b55c943 to 162362e Compare May 27, 2020 13:54
@ElizabethOkerio ElizabethOkerio force-pushed the fix/migrate_odata_client_tests_to_xunit branch from 162362e to 9790cf2 Compare June 15, 2020 19:52
@ElizabethOkerio ElizabethOkerio added the Ready for review Use this label if a pull request is ready to be reviewed label Jun 16, 2020
@xuzhg
Copy link
Member

xuzhg commented Jun 18, 2020

<PackageReference Include="FluentAssertions" Version="4.1.0" />

shall we remove "FluentAssertions" since we move to xunit.net


Refers to: test/EndToEndTests/Tests/Client/Build.Desktop/Microsoft.Test.OData.Tests.Client.csproj:37 in 14d8b45. [](commit_id = 14d8b45, deletion_comment = False)

@xuzhg
Copy link
Member

xuzhg commented Jun 18, 2020

    }

please move to top


Refers to: test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/Materialization/MaterializationPolicyUnitTests.cs:36 in 14d8b45. [](commit_id = 14d8b45, deletion_comment = False)

@xuzhg
Copy link
Member

xuzhg commented Jun 18, 2020

<PackageReference Include="FluentAssertions" Version="2.0.0.0" />

can we remove this dependency?


Refers to: test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Microsoft.OData.Client.TDDUnitTests.csproj:26 in 14d8b45. [](commit_id = 14d8b45, deletion_comment = False)

Copy link
Member

@xuzhg xuzhg left a comment

Choose a reason for hiding this comment

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

:shipit:

@xuzhg
Copy link
Member

xuzhg commented Jun 18, 2020

please rebase to latest master, make build pass and merge with squash

Copy link

@odero odero left a comment

Choose a reason for hiding this comment

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

Non blockers but worth reviewing

@@ -105,7 +105,7 @@ public async Task SaveChangesTest()
customers.Add(c2);

var dataServiceResponse = await context.SaveChangesAsync(SaveChangesOptions.BatchWithIndependentOperations | SaveChangesOptions.UseRelativeUri);
Assert.Equal((dataServiceResponse.First() as ChangeOperationResponse).StatusCode, 201);
Assert.Equal(201, (dataServiceResponse.First() as ChangeOperationResponse).StatusCode);
Copy link

Choose a reason for hiding this comment

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

While you're here, you might want to use the HttpStatusCode Enum, for readability
https://docs.microsoft.com/en-us/dotnet/api/system.net.httpstatuscode

#else
using Microsoft.VisualStudio.TestTools.UnitTesting;

#endif
#if !PORTABLELIB && !SILVERLIGHT
Copy link

Choose a reason for hiding this comment

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

Possible to remove these portablelib silverlight references while you are doing this?

Assert.AreEqual(string.Format("{0}.AddressType", TestModelNameSpace), annotationOnHomeAddress.Name);
Assert.AreEqual("Home", (annotationOnHomeAddress.Value as ODataPrimitiveValue).Value);
Assert.Equal(string.Format("{0}.AddressType", TestModelNameSpace), annotationOnHomeAddress.Name);
Assert.Equal("Home", (annotationOnHomeAddress.Value as ODataPrimitiveValue).Value);

// TODO : Fix #625
Copy link

Choose a reason for hiding this comment

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

Could you check on this issue and probably close if it's been resolved?

public void ServiceOperationFeedQuery()
{
var contextWrapper = this.CreateContext();
var queryResult = contextWrapper.Execute<Customer>(new Uri(this.ServiceUri.OriginalString + "/GetSpecificCustomer?Name='enumeratetrademarkexecutionbrfalsenesteddupoverflowspacebarseekietfbeforeobservedstart'"), "GET", true).ToArray();
Assert.AreEqual(1, queryResult.Count(), "Expected a single Customer return");
Assert.Equal(1, queryResult.Count());
Copy link

Choose a reason for hiding this comment

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

Suggested change
Assert.Equal(1, queryResult.Count());
Assert.Single(queryResult);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xunit recommends the use of Single for a collection with a count of 1.

Copy link

@odero odero Jun 25, 2020

Choose a reason for hiding this comment

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

Isn't that what this is? Or does it have to be a List (with a .Count prop) as opposed to just an IEnumerable?

}
}
}

private static void AssertODataCollectionValueAreEqual(ODataCollectionValue expectedCollectionValue, ODataCollectionValue actualCollectionValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change Method name also (doesnt harm though)? I guess you would have done a Cntrl+H ?

@ElizabethOkerio ElizabethOkerio force-pushed the fix/migrate_odata_client_tests_to_xunit branch from 14d8b45 to 2828009 Compare June 24, 2020 18:38
@Sreejithpin Sreejithpin merged commit 1e661db into OData:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review Use this label if a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants