Skip to content

Change RestBaseResponse property bag bag to setters from init #1805

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

Conversation

kendallb
Copy link
Contributor

Description

These need to not be init only, because it can be quite useful to return mocked responses and it these are all init only, we cannot adjust them after creating a new instance.

I guess you could argue we should mock the class instead, so I could try that also, but is there any reason not to allow these properties to be setters so its easier for faking responses from lower level services?

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)

Sorry, something went wrong.

@kendallb
Copy link
Contributor Author

I cannot run that Twitter test, as I don't have keys. How do I set it up to run that test?

I suspect it's failing because Twitter is down or something?

@kendallb
Copy link
Contributor Author

Oh, its not twitter, that is skipped. I am not sure what is failing? It passes for me?

Passed! - Failed: 0, Passed: 81, Skipped: 7, Total: 88, Duration: 1 m 45 s - RestSharp.Tests.Integrated.dll (net6.0)
Error: Process completed with exit code 1.

@kendallb
Copy link
Contributor Author

I did get a transient failure on Task_Handles_Non_Existent_Domain, but I ran it again and it worked. So I wonder if it's the same issue.

@kendallb kendallb force-pushed the feature/allow-settings-response-properties branch from 65349ef to 7646b2a Compare March 18, 2022 18:11
@kendallb
Copy link
Contributor Author

I guess you could argue we should mock the class instead, so I could try that also, but is there any reason not to allow these properties to be setters so its easier for faking responses from lower level services?

Bah, that does not work as you cannot mock anything on a real class unless the values are virtual so the only way to mock this would be via an interface (which is gone), or by creating a mock real response property bag and stuffing values into it.

…urn mocked responses and it these are all init only, we cannot adjust them after creating a new instance.

I guess you could argue we should mock the class instead, so I could try that also, but is there any reason not to allow these properties to be setters so its easier for faking responses from lower level services?
@kendallb kendallb force-pushed the feature/allow-settings-response-properties branch from a526aa3 to 23b0db8 Compare March 18, 2022 18:21
@alexeyzimarev
Copy link
Member

These need to not be init only, because it can be quite useful to return mocked responses and it these are all init only, we cannot adjust them after creating a new instance.

The way to work with records is to use something:

var newResp = response with { Content = "blah" };

The reason for those props to be init-only is to enforce immutability. The response should not be changed after it's created. I only see an issue with init that it doesn't work with older compiler versions, but that would be fixed by making them internal set. Could you give an example of the mocked response where you need to manipulate its content?

@kendallb
Copy link
Contributor Author

Sure, it's code one of my guys wrote to test some services he wrote. Just to test the basic code before it gets to the REST calls, which we consider a black box. So the code is something similar to this:

            var response = new RestResponse<List<ExplodedViews>> {
                StatusCode = HttpStatusCode.NotFound,
            };

So the two problems with that is that a) we are on VS2019 currently and b) if it becomes an internal setter, we can't do the same. I tried to mock the response, but since the response is both a) not an interface and b) does not have virtual functions, you can't mock it.

This is one instance where the IRestResponse interface was actually quite useful for simple unit testing. The tests in question are really just make sure all the error handling works as expected.

@alexeyzimarev alexeyzimarev merged commit 652a4c4 into restsharp:dev Mar 22, 2022
@kendallb kendallb deleted the feature/allow-settings-response-properties branch November 8, 2022 15:32
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.

None yet

2 participants