Playwright refactoring + api project #134
Conversation
|
|
||
| if (!IsAbsoluteUrl(url)) | ||
| { | ||
| url = ConfigManager.GetConfig<Config>().ApiHost + url; |
There was a problem hiding this comment.
It makes sense to rewrite IsAbsoluteUrl to something like GetAbsoluteUrl since it's use throughout the code. Additionally, you can return Uri from this method that will be well-formed and wouldn't require using string concatenation
| } | ||
|
|
||
| [When("user sends a \"(.*)\" request to \"(.*)\" url with the json:")] | ||
| public HttpResponseMessage UserSendsHttpRequestWithParameters(string httpMethod, string url, string jsonToSend) |
There was a problem hiding this comment.
I'm sure it's a bad idea to write json as string inside the .feature file, it makes file bigger, less readable and harder to format, because not Visual Studio, nor Rider can successfully format both .feature file and json inside it at once.
There was a problem hiding this comment.
If JSON is not very long it looks fine for me. I've added a step and an example using a file as a JSON source.
| [Then("response attachment filename is \"(.*)\"")] | ||
| public void ThenResponseAttachmentFilenameIs(string filename) | ||
| { | ||
| if (_apiContext.Response is null) throw new NullReferenceException("Http response is empty."); |
There was a problem hiding this comment.
I don't think that exception type should be nullref here, it looks more like a HttpResponseException
There was a problem hiding this comment.
In this case, NullReferenceException throws if an object is null so I think it is fine. The root cause of the issue is that the request wasn't sent so it more test consistency issue.
There was a problem hiding this comment.
In C# NullReferenceException is generally thrown when user tries to access null instead of valid value. Here no one tries to access it, so logically it seems right to use other kind, maybe not NullReferenceException, but definitely not NullReferenceException
| [Given("response attachment is saved as \"(.*)\"")] | ||
| public void GivenResponseAttachmentIsSavedAs(string filePath) | ||
| { | ||
| if (_apiContext.Response is null) throw new NullReferenceException("Http response is empty."); |
| var fullPath = Path.GetFullPath(Path.Join(Directory.GetCurrentDirectory(), filePath)) | ||
| .NormalizePathAccordingOs(); | ||
|
|
||
| var directoryPath = Path.GetDirectoryName(fullPath); |
There was a problem hiding this comment.
Do we really need this variable?
Why not use fullPath instead?
There was a problem hiding this comment.
Looks like yes. The logic is to check whether a directory exists and create if not.
There was a problem hiding this comment.
It is not clear here what could be written in filePath - full path or relative path? Because of that further manipulations with this value are hard to understand
| FailJTokensAssertion(actualJTokens, expectedJTokens, "Elements count mismatch."); | ||
| } | ||
|
|
||
| if (!IsJTokenListsSimilar(expectedJTokens, actualJTokens)) |
There was a problem hiding this comment.
This method name can be misinterpreted
IsJTokenListItemsAreTheSame might be better
|
|
||
| private List<JToken> GetActualJTokensFromResponse(string jsonPath) | ||
| { | ||
| if (_apiContext.Response is null) throw new NullReferenceException("Http response is empty."); |
There was a problem hiding this comment.
Same as mentioned
| { | ||
| if (!expected.Trim().StartsWith("[")) | ||
| { | ||
| expected = expected.Insert(0, "["); |
There was a problem hiding this comment.
Interpolation here will look better and easier to read
expected = $"[{expected}"
| @@ -14,7 +14,7 @@ public class ClickBinding | |||
| /// <example>When user clicks on "Test" button</example> | |||
| [Given(@"user clicked on ""(.+?)""")] | |||
| [When(@"user clicks on ""(.+?)""")] | |||
| public async Task ClickOnElement(IWebElementWrapper element) | |||
| public async Task ClickOnElement(WebElementWrapper element) | |||
There was a problem hiding this comment.
Why change interface to concrete implementation?
There was a problem hiding this comment.
The framework is intended to be simple and light. So without the abstract layer, it will be more straightforward.
There was a problem hiding this comment.
And how do you intend to override default wrappers that you have in new projects in case of compatibility issues?
There was a problem hiding this comment.
To create a new one or edit existing ones. Since we don't have a NuGet package the solution will be simple cloned and customized.
No description provided.