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

New methods #92

Merged
merged 6 commits into from
Sep 23, 2022
Merged

New methods #92

merged 6 commits into from
Sep 23, 2022

Conversation

pseudodream
Copy link
Contributor

Added a method to get the list of all imposters

@mattherman
Copy link
Owner

I'm hoping to review this sometime this week. Apologies for the long turnaround.

/// </summary>

/// <returns>The list of logs</returns>
Task<List<Log>> GetLogsAsync(CancellationToken cancellationToken=default);
Copy link
Owner

Choose a reason for hiding this comment

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

Update this to return Task<IEnumerable<Log>>

/// </summary>
/// <returns>The list of retrieved imposters</returns>

Task<List<RetrievedImposters>> GetImpostersAsync(CancellationToken cancellationToken = default);
Copy link
Owner

Choose a reason for hiding this comment

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

Update this to return Task<IEnumerable<RetrievedImposters>>

@@ -15,5 +19,9 @@ internal interface IRequestProxy
Task<RetrievedHttpsImposter> GetHttpsImposterAsync(int port, CancellationToken cancellationToken = default);
Task<RetrievedSmtpImposter> GetSmtpImposterAsync(int port, CancellationToken cancellationToken = default);
Task DeleteSavedRequestsAsync(int port, CancellationToken cancellationToken = default);

Task<List<RetrievedImposters>> GetImpostersAsync(CancellationToken cancellationToken = default);
Copy link
Owner

Choose a reason for hiding this comment

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

Update this to return Task<IEnumerable<RetrievedImposters>>


Task<List<RetrievedImposters>> GetImpostersAsync(CancellationToken cancellationToken = default);
Task<Home> GetEntryHypermediaAsync(CancellationToken cancellationToken = default);
Task<List<Log>> GetLogsAsync(CancellationToken cancellationToken = default);
Copy link
Owner

Choose a reason for hiding this comment

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

Update this to return Task<IEnumerable<Log>>



[JsonProperty("Timestamp")]
public string Timestamp { get; internal set; }
Copy link
Owner

Choose a reason for hiding this comment

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

You should be able to make this a DateTime and it will deserialize it automatically.

using (var response = await _httpClient.GetAsync("/logs", cancellationToken).ConfigureAwait(false))
{
await HandleResponse(response, HttpStatusCode.OK, $"Failed to get the logs ",
(message) => new Exception(message)).ConfigureAwait(false);
Copy link
Owner

Choose a reason for hiding this comment

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

Don't provide an exceptionFactory here. It will wrap it in a MountebankException by default.

{

await HandleResponse(response, HttpStatusCode.OK, $"Failed to retrieve the list of imposters ",
(message) => new ImposterNotFoundException(message)).ConfigureAwait(false);
Copy link
Owner

Choose a reason for hiding this comment

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

Don't provide an exceptionFactory here. ImposterNotFoundException is not really accurate for this request. Let it default to MountebankException.


private async Task<List> GetImpostersAsync<List>(CancellationToken cancellationToken = default)
{
using (var response = await _httpClient.GetAsync($"{ImpostersResource}", cancellationToken).ConfigureAwait(false))
Copy link
Owner

Choose a reason for hiding this comment

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

I know it wasn't added by you, but could you delete and inline the ImpostersResource field? It's pretty unnecessary and using string literals in these methods lets you tie them back to the actual API more easily.

Comment on lines +18 to +21
await Client.DeleteAllImpostersAsync();

Client.Imposters.Add(new HttpImposter(123, null));
Client.Imposters.Add(new HttpImposter(456, null));
Copy link
Owner

Choose a reason for hiding this comment

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

This is not needed. All you need to do is setup the mock as you did below.


var list=await _client.GetImpostersAsync();

Assert.IsNotNull(list);
Copy link
Owner

Choose a reason for hiding this comment

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

Please add an assert for the expected count.

@mattherman mattherman merged commit 9848f0d into mattherman:master Sep 23, 2022
mattherman added a commit that referenced this pull request Sep 23, 2022
mattherman added a commit that referenced this pull request Sep 23, 2022
mattherman added a commit that referenced this pull request Sep 26, 2022
mattherman added a commit that referenced this pull request Sep 26, 2022
@mattherman
Copy link
Owner

These changes have been published in v5.0.0-rc8.

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