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

ScanAPI spec from an OpenAPI specification #206

Closed
wants to merge 11 commits into from
Closed

ScanAPI spec from an OpenAPI specification #206

wants to merge 11 commits into from

Conversation

abreumatheus
Copy link
Member

Refers to: [#12 ]

Added

  • Command convert, to convert a OpenAPI JSON file to a ScanAPI YAML friendly file;

Changed

  • Command scanapi to scanapi run;
  • Updated documentation to include changes and the new feature;

@abreumatheus
Copy link
Member Author

abreumatheus commented Jul 7, 2020

I think the only thing missing is to write the tests for this. I didn't do it because i'm not familiar with how you're writing them.
Also, it's not passing the lint check, but i already double checked this for formatting problems, but didn't seem to work :/.

@camilamaia
Copy link
Member

Hi @abreumatheus thanks for the PR!

About the tests, you can create a file called tests/unit/test_openapi_to_yaml.py and create the tests using pytest.

Usually, what I do, is to create classes to give some context about the tests, something inspired in BDD:

class TestFileName: # example: TestRegistration
   class TestFunctionName: # example TestRegisterAccount
      class TestContext: # example TestWhenDataIsIncomplete
         def test_expect_behaviour(self): # example test_should_return_422
            pass

but don't worry to use it if you don't feel comfortable.

@abreumatheus
Copy link
Member Author

@camilamaia Thank you for the explanation, makes a lot more sense now hahaha. I use a different aproach at work with unittest. Will try here :)

@camilamaia
Copy link
Member

@abreumatheus no problem! If you feel stuck or struggling too much, please let me know! We can do a pair or something like this.

@abreumatheus
Copy link
Member Author

abreumatheus commented Jul 8, 2020

Hey, @camilamaia.
Just pushed 100% test coverage for the openapi_to_yaml.py file, but i'm having a bit of a challenge while trying to mock it in the main.py file tests using pytest. Could you try to find what am i doing wrong?

Test:

def test_should_call_openapi_to_yaml(self, mocker):
    runner = CliRunner()
    mock_openapi_to_yaml = mocker.patch("scanapi.main.openapi_to_yaml")
    result = runner.invoke(convert, '../data/openapi.json')
    os.remove('./api.yaml')

    assert result.exit_code == 0

    mock_openapi_to_yaml.assert_called_once_with('../data/openapi.json')

Error:

self = <unittest.mock._patch object at 0x04852F58>

    def get_original(self):
        target = self.getter()
        name = self.attribute
    
        original = DEFAULT
        local = False
    
        try:
            original = target.__dict__[name]
        except (AttributeError, KeyError):
            original = getattr(target, name, DEFAULT)
        else:
            local = True
    
        if name in _builtins and isinstance(target, ModuleType):
            self.create = True
    
        if not self.create and original is DEFAULT:
>           raise AttributeError(
                "%s does not have the attribute %r" % (target, name)
            )
E           AttributeError: <Group main> does not have the attribute 'openapi_to_yaml'

local      = False
name       = 'openapi_to_yaml'
original   = sentinel.DEFAULT
self       = <unittest.mock._patch object at 0x04852F58>
target     = <Group main>

Ps: I pushed the failing test, if you wanna try it yourself.

@abreumatheus
Copy link
Member Author

@camilamaia Any news on this? Sorry to bother, really wanna finish this feature to see it working live 😁.

@camilamaia
Copy link
Member

@camilamaia Any news on this? Sorry to bother, really wanna finish this feature to see it working live 😁.

Hi @abreumatheus ! I was super busy in the last week, sorry. But I will check it in the beginning of this week

@camilamaia
Copy link
Member

@abreumatheus I got the problem! The test was trying to get the group instead of the module when mocking, because they have the same name "main".

What I did and worked was to change the name of the file from main.py to __main__.py:

4d0a9a5

I also changed the relative paths like this, because they did not work on my machine.

I invited you to the organization, I don't know if you saw it. Cause if you are in the org, you don't need to use a forked project and can grab from the origin branch i've just pushed to.

openapi_to_yaml('../data/openapi.json')

assert os.path.exists('./api.yaml')

Copy link
Member

Choose a reason for hiding this comment

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

Can we have an assert here for the expected result? We could create a file in the data folder with the result:

api:
    endpoints:
    -   name: Uber API
        path: https://api.uber.com/v1
        requests:
        -   name: Product Types
            path: /products
            method: get
        -   name: Price Estimates
            path: /estimates/price
            method: get
        -   name: Time Estimates
            path: /estimates/time
            method: get
        -   name: User Profile
            path: /me
            method: get
        -   name: User Activity
            path: /history
            method: get

and assert the content matches. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@abreumatheus I was noticing that the query params, for example, are not in the expected result. Is this intentional? Does it work for headers, body and others http methods than get too? If not, we can create more issues for them, no problem, I just want to understand the context here. Thanks!!

Copy link
Member Author

Choose a reason for hiding this comment

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

@camilamaia Will assert the expected result. I really forgot about the query/path parameters and headers hahaha. I was analyzing it here and i have to work on a couple more things too, like ofering support for the new 3.0 version of OpenAPI that, among some differences, can also come on YAML format now. I was thinking about closing this PR, since it will require a lot of new things and open another one when it's all trully ready and beautiful 😄 . What do you think?

Edit: It seems i don't have permission to push branches to the repository, i'm also working on the json render, so i will push to my fork.

Copy link
Member

Choose a reason for hiding this comment

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

@abreumatheus I added you to the team which can push stuff, sorry! I needed you to accept the invite first. But it should be all set now 👌 Please, let me know if you keep having problems.

What you can do is to work in a branch as a draft. Because this is getting in a good shape. Actually, the way you prefer works for me!

Good catch about the OpenAPI 3.0 👀 I was forgetting it too!

@camilamaia
Copy link
Member

@abreumatheus Did the trick of changing the main.py file name work for you?

@abreumatheus
Copy link
Member Author

@abreumatheus Did the trick of changing the main.py file name work for you?

Yes! It did!

@scanapi scanapi locked and limited conversation to collaborators Jul 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants