Reuse path variable for bodyfile #176
Conversation
|
Hey @eloo-abi! Thanks for your contribution 💟 However, before moving forward, I'd like to see some tests that show the expected behavior in different scenarios (e.g. no variables at all, a variable with a single value, a variable with multiple values, multiple variables, when a non-defined variable is used, etc). |
joanlopez
requested review from
joanlopez and
aperezg
and removed request for
joanlopez
|
Hi,
this is covered by the existing tests already as the implementation is not going to change anything here
will be added
not sure what this is, can you give an example of a variable with multiple values?
will be added
it behaves like a wrong manually entered path as the implementation is not doing anything special, just adjust the path to search for a file |
Yes, please, ignore the case of multiple values for a single variable (I left a comment in the issue). |
Merge in OSSC/killgrave from feature/friendsofgoGH-175-reuse-path-variable-for-bodyfile to feat/friendsofgoGH-175-reuse-path-variable-for-bodyfile * commit '18e28e7f7c16c2a4fc670f2aae6b744a82c1be51': remove unused line test: add tests for variables in body file
|
tests added i was not sure where or how to add to i have added it similar to other |
|
@joanlopez thanks |
internal/server/http/test/testdata/imposters_variables/responses/gopher_1_2_response.json
Show resolved
Hide resolved
| expectedBodyPath string | ||
| statusCode int | ||
| }{ | ||
| {"valid imposter with id 1 in path", imposters[0], "/gophers/1", responseId1, http.StatusOK}, |
There was a problem hiding this comment.
Can you add a test case where there's a value defined in the bodyFile path but not in the endpoint, please? Just to reflect what's the expectation in such case.
There was a problem hiding this comment.
for sure..
but this is the same behaviour as right now if the file could not be found
"200" with empty body
Hey @eloo-abi! Sorry for the delay, I've been quite busy during recent weeks. Thanks! 🙇🏻 |
Merge in OSSC/killgrave from feature/friendsofgoGH-175-reuse-path-variable-for-bodyfile to feat/friendsofgoGH-175-reuse-path-variable-for-bodyfile * commit 'c9dbb4b7afa8c138d678ba338d7f4be397297034': fix: adjust test data and handle errors
|
@joanlopez everything should be addressed :) |
|
@joanlopez did you have already time to have a look? |
Been AFK for some time during September, but planning to review it during October for sure! 🙇🏻 |
| responseId2 := "test/testdata/imposters_variables/responses/gopher_2_response.json" | ||
| responseId1Variable1 := "test/testdata/imposters_variables/responses/gopher_1_1_response.json" | ||
| responseId1Variable2 := "test/testdata/imposters_variables/responses/gopher_1_2_response.json" | ||
| responseId1WithoutVariable := "test/testdata/imposters_variables/responses/gopher_1_without_variable_response.json" |
There was a problem hiding this comment.
Nit; I think this could be refactored in a way so we use the file contents in the test (instead of the file path), so we don't need an empty file for the case where there's no data, but it's not critical/blocking for merging this PR.
|
@joanlopez if its not a blocker please merge this PR as it takes already a very long time for such a small change |
Yeah, sure! I'm waiting for @aperezg to give his 👍🏻, which I hope we get in the next few days, it's alright from my side! 🙇🏻 |
|
@joanlopez @aperezg any updates here? |
Hi,
this PR enables the usage of the variables defined in the imposters to be reused in the bodyFile path.
So we can use "dynamic" responses for different paths easily.
Fixes: #175