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

Chore(Tests): CNX-8640 core integration test project #3130

Merged
merged 8 commits into from
Jan 12, 2024

Conversation

JR-Morgan
Copy link
Member

@JR-Morgan JR-Morgan commented Jan 10, 2024

  • Resolved naming violations in Integration test projects
  • Updated editor config to warn when using the old nunit functions
  • Updated both Unit and integration tests to use the new nunit functions
  • Removed ConfigureAwaits from test project:
    • My reasoning is. They aren't needed, don't provide value, just adds visual clutter.
    • Happy to discuss and revert if prefered.

@JR-Morgan JR-Morgan requested a review from AlanRynne January 10, 2024 22:42
@JR-Morgan JR-Morgan added the core issues related to the .net sdk. label Jan 10, 2024
@JR-Morgan JR-Morgan modified the milestones: 2.x, 2.18 Jan 10, 2024
@JR-Morgan JR-Morgan changed the title Chore(Tests): CNX 8640 core integration test project Chore(Tests): CNX8640 core integration test project Jan 10, 2024
@JR-Morgan JR-Morgan changed the title Chore(Tests): CNX8640 core integration test project Chore(Tests): CNX-8640 core integration test project Jan 10, 2024
@JR-Morgan JR-Morgan marked this pull request as ready for review January 11, 2024 01:04
@AlanRynne AlanRynne self-assigned this Jan 11, 2024
Copy link
Contributor

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

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

No complaints, just a question 👇🏼 Approving anyways

Comment on lines -67 to -69
_client.MaybeThrowFromGraphQLErrors(new GraphQLRequest(), new GraphQLResponse<string>());
// We're just checking that the prev function didn't throw
Assert.True(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh WTH 🤣

@@ -33,7 +33,7 @@ public void Receive_OperationCanceled_ExceptionThrown(string id)
ctc.Cancel();

MemoryTransport emptyTransport2 = new();
Assert.ThrowsAsync<OperationCanceledException>(async () =>
Assert.CatchAsync<OperationCanceledException>(async () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is CatchAsync the recommended pattern to assert an exception type here? Asking out of ignorance

Copy link
Member Author

@JR-Morgan JR-Morgan Jan 12, 2024

Choose a reason for hiding this comment

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

ThrowsAsync tests if the method throws the EXACT type
CatchAsync allows subtypes

In this case, the spirit of the test was to make sure it an OperationCancelledException, we don't care if its a subtype like TaskCancelledException, so I after learning of the existence of CatchAsync, I decided to update the functions where we would be ok with subtypes.

@AlanRynne AlanRynne merged commit a4966cf into dev Jan 12, 2024
30 checks passed
@AlanRynne AlanRynne deleted the CNX-8640-Core-Integration-Test-Project branch January 12, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core issues related to the .net sdk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants