Conversation
* Fix the --browser option The problem was that webbrowser.open expects a URI and doesn't work with relative local paths. Solution: use pathlib to convert reporter.output_path to a URI using pathlib.Path(reporter.output_path).resolve().as_uri(). * Add a changelog entry
f13ac53 to
1d9cabf
Compare
26511c2 to
775c04a
Compare
camilamaia
left a comment
There was a problem hiding this comment.
Awesome PR @gabrieltron! This was a super desired feature that we were trying to deliver for a long time. Thank you very much!
Also, pretty elegant solution 🌟
I left only one small change request, let me know what do you think :)
| @mark.context("when key exists in endpoint node") | ||
| @mark.it("should search endpoint node") | ||
| @mark.it("should return the value in endpoint node") | ||
| def test_should_return_list(self, spec_evaluator): |
There was a problem hiding this comment.
| def test_should_return_list(self, spec_evaluator): | |
| def test_should_return_list(self, mocker, spec_evaluator): |
| def test_should_return_list(self, spec_evaluator): | ||
| key = "requests" | ||
| expected_value = "value" | ||
| with mock.patch( |
There was a problem hiding this comment.
| with mock.patch( | |
| with mocker.patch( |
| @@ -1,4 +1,5 @@ | |||
| from pytest import fixture, mark, raises | |||
| from unittest import mock | |||
There was a problem hiding this comment.
We don't need to import mock from unittest because we use the pytest-mock library. It already provides a mocker fixture which is a thin-wrapper around the patching API provided by the mock package:
| from unittest import mock |
The two next suggestions show how to do it using mocker fixture.
There was a problem hiding this comment.
Yup! Fixed it. I forgot that the scope of a fixture is a function by default, so now I just used the existing mock from the fixture instead 😅
| @@ -0,0 +1,29 @@ | |||
| from pytest import mark | |||
There was a problem hiding this comment.
@gabrieltron can we have a test in which we have vars with the same name for both parent and child? To check that the inner value should be the one returned: the child's one.
For example:
endpoints:
- name: snippets-api
path: http://demo.scanapi.dev/api/v1/
headers:
Content-Type: application/json
vars:
my_var: one
requests:
- name: health
path: health?${my_var}
endpoints:
- name: endpoint
vars:
my_var: two
requests:
- name: another health
path: health?${my_var}Request one should be: GET http://demo.scanapi.dev/api/v1/health?one
Request two should be: GET http://demo.scanapi.dev/api/v1/health?two
There was a problem hiding this comment.
Nice, I hadn't thought of that scenario. I changed the method so it only propagates the new vars.
I also changed it so it only writes if the variable doesn't exist. On the downside, this implies that there's no way of update global variables. On the upside, it further protects the variables already declared in the parents. Do you think this is a good approach? Or should request nodes be able to update global variables?
775c04a to
31035fd
Compare
31035fd to
5bb3df8
Compare
|
Any news on that? I'd appreciate feedback, even if it's that we should hold this up for now hahaha |
There was a problem hiding this comment.
Thank you for supporting ScanAPI, and congratulations on your first contribution! A project committer will shortly review your contribution.
In the mean time, if you haven't had a chance please skim over the First Pull Request Guide which all pull requests must adhere to.
We hope to see you around!
sorry for the super late response, taking a look into this. If you are still up for it let me know else I can take it forward from the work you have already done. Thank You 💜 Added a new PR alltogether and added you as a co-author. |
Description
Allow global variables. When
spec_varsis updated after performing a call, instead of updating thespec_varsof just the endpoint node, recursively update all parent endpoint nodes. When evaluating an expression, instead of searching for the variable inspec_varsof only the request's endpoint node, search all parent request nodes recursively.Motivation behind this PR?
#265
What type of change is this?
Feature
Checklist
Issue
Closes #265