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

Add fudament for API based on connexion #8149

Merged
merged 4 commits into from
Jun 2, 2020

Conversation

mik-laj
Copy link
Member

@mik-laj mik-laj commented Apr 5, 2020

Close: #8109


Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@mik-laj mik-laj force-pushed the aip-XXXXX-api-poc branch 2 times, most recently from 0ddd5bf to 32ddbc2 Compare April 5, 2020 01:17
@mik-laj mik-laj changed the title Add fudament for API based on cconnexion Add fudament for API based on cconnexion [WIP] Apr 5, 2020
@mik-laj mik-laj changed the title Add fudament for API based on cconnexion [WIP] Add fudament for API based on connexion [WIP] Apr 5, 2020
@mik-laj mik-laj added the area:API Airflow's REST/HTTP API label May 18, 2020
@mik-laj mik-laj force-pushed the aip-XXXXX-api-poc branch 14 times, most recently from 6773bd3 to 0e73b0c Compare May 21, 2020 21:26
@mik-laj mik-laj force-pushed the aip-XXXXX-api-poc branch 4 times, most recently from da95c36 to 54bde13 Compare June 1, 2020 21:36
@mik-laj mik-laj force-pushed the aip-XXXXX-api-poc branch 2 times, most recently from 9afa53b to 74ae716 Compare June 2, 2020 09:58
Comment on lines +24 to +31
class TestConnectionEndpoint(unittest.TestCase):
@classmethod
def setUpClass(cls) -> None:
super().setUpClass()
cls.app = app.create_app(testing=True) # type:ignore

def setUp(self) -> None:
self.client = self.app.test_client() # type:ignore
Copy link
Member

Choose a reason for hiding this comment

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

This class code is repeated in each test file. Also we have http_client fixture which also provides the app 😸

Copy link
Member Author

@mik-laj mik-laj Jun 2, 2020

Choose a reason for hiding this comment

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

Yes. There are so many repetitions in this code because it is a skeleton. I hope that soon this code will be more different. I didn't want to introduce 3-level inheritance here, because it would complicate the tests excessively. This is ok if the code is repeated because it makes the code much simpler and less complex. Now each test file is not dependent on anything and each person can work independently.
Here is an article about it: https://testing.googleblog.com/2019/12/testing-on-toilet-tests-too-dry-make.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

And what about the fixture? Should it stay or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed fixture: 6c765df
Thanks.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM

@mik-laj mik-laj changed the title Add fudament for API based on connexion [WIP] Add fudament for API based on connexion Jun 2, 2020
@mik-laj mik-laj merged commit 67379d1 into apache:master Jun 2, 2020
@mik-laj mik-laj deleted the aip-XXXXX-api-poc branch June 2, 2020 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:CLI area:dev-tools area:webserver Webserver related Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Basic integration Airflow and connexion
3 participants