-
Notifications
You must be signed in to change notification settings - Fork 18
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
ENG-1358 Set up backend integration tests & include in Github Actions #214
ENG-1358 Set up backend integration tests & include in Github Actions #214
Conversation
Hi @kenxu95 made an initial draft of the backend integration test suite. Had some questions & would appreciate your feedback:
|
|
working-directory: integration_tests/backend | ||
env: | ||
SERVER_ADDRESS: localhost:8080 | ||
ADAPTER: http:// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the requests.get
url
…ub.com:aqueducthq/aqueduct into eng-1358-set-up-backend-integration-tests-include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is really hard for me to read and understand, can we chat in person? Maybe we can pair a little :)
…ub.com:aqueducthq/aqueduct into eng-1358-set-up-backend-integration-tests-include
Action items:
|
…ub.com:aqueducthq/aqueduct into eng-1358-set-up-backend-integration-tests-include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Love the documentation. A few more suggestions - just ping me when they're ready :)
try: | ||
r = requests.get("http://"+url, headers=headers) | ||
except: | ||
raise Exception(f"Cannot connect to {url}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to use the same logical structure that we use in the api_client.py on the SDK? That way its 1) more consistent across our tests and 2) avoiding having nested try-except clauses which is a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm that might be better - how about we add a method on api_client called ui_and_server_address()
, which will return the {prefix}{server_address} as a string that you can join with the routes? This'll be a useful internal method anyways - see generate_ui_url()
in the SDK's utils.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your suggestion of keeping the logic in one place, this just wraps it a little more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- using existing
api_client._construct_full_url
in tests - created
api_client._construct_base_url
api_client._construct_full_url
usesapi_client._construct_base_url
- made the arg
use_http
optional forapi_client._construct_full_url
andapi_client._construct_base_url
(ifuse_http=None
, useself.use_http
) - refactor
generate_ui_url
to useapi_client._construct_full_url
- clean url in
api_client
soSERVER_ADDRESS=localhost:8080/
works (would error out before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Erroneous package-lock.json changes? I can stamp after those are fixed. Also dropped a few simplifying suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for powering through this!
…#216) * Add route url * first draft * Draft 2 -- custom sql * SQL return all operators * distinct load list * lint * Custom response struct * package-lock update * lint * table_artifact merge conflict * lint * black * Right version of black * ENG-1358 Set up backend integration tests & include in Github Actions (#214)
Describe your changes and why you are making these changes
Suite of tests for pure endpoint testing. Checking reads in a stricter fashion.
Testing
Successfully ran Integration Tests Github Actions workflow with these changes: https://github.com/aqueducthq/aqueduct/actions/runs/2667299271
Related issue number (if any)
Eng 1358
Eng 1345 (#216): The changes being tested
Checklist before requesting a review
python3 scripts/run_linters.py -h
for usage).run_integration_test
: Runs integration testsskip_integration_test
: Skips integration tests (Should be used when changes are ONLY documentation/UI)