-
Notifications
You must be signed in to change notification settings - Fork 38
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
Mock s3 test #231
Mock s3 test #231
Conversation
.github/workflows/ci.yml
Outdated
python3 -m pip install h11 trio | ||
- name: Build ${{ env.PACKAGE_NAME }} | ||
run: | | ||
cd ./tests/mock_s3_server/ | ||
Start-Process -NoNewWindow python .\mock_s3_server.py | ||
python3 -c "from urllib.request import urlretrieve; urlretrieve('${{ env.BUILDER_HOST }}/${{ env.BUILDER_SOURCE }}/${{ env.BUILDER_VERSION }}/builder.pyz?run=${{ env.RUN }}', 'builder.pyz')" | ||
python3 builder.pyz build -p ${{ env.PACKAGE_NAME }} --cmake-extra=-DENABLE_MOCK_SERVER_TESTS=ON |
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.
debatable: we could try to set up the mock server in EVERY CI check (instead of just these 3) if we set it up via a custom builder script.
This is how I did PKCS#11 tests. Check out aws-c-io/.builder/actions/pkcs11_test_setup.py. It tries to install dependencies, and sets the appropriate environment variables to run the tests if it succeeds (if it can't install the dependencies, then it just spits out a warning and doesn't set the environment variables). You can launch the background process via subprocess.Popen()
tests/s3_mock_server_tests.c
Outdated
/* Test that the remote max concurrent streams setting hit */ | ||
TEST_CASE(multipart_upload_mock_server) { |
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.
It seems like this is a "success" test, but comment says something about "max concurrent streams"
print("Mock server failed to setup, skip the mock server tests.") | ||
return |
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 am not sure which is better, error out or return..
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.
Is it failing to install anywhere? If it works everywhere, then make it an error
If we just return: there's a chance that setup starts failing everywhere, but we never realize because it's a silent warning
If we make it an error: then you can end up spending waaaaay too much time trying to get it working on esoteric/old platforms like manylinux1.
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.
current it's not failing from anywhere. But, two weeks ago, I think github ubuntu 20 image was failing to install trio for "I don't know" reason. Let's make it fail and heck around the image that doesn't support it.
if os.name == 'nt': | ||
python_execute = "python" | ||
else: | ||
python_execute = "python3" | ||
|
||
# install dependency for mock server | ||
result = self.env.shell.exec(python_execute, | ||
'-m', 'install', 'h11', 'trio') |
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.
Use the string "{python}"
Builder will substitute the correct python path for this image
if result.returncode != 0: | ||
print( | ||
"Mock server failed to setup, skip the mock server tests.", file=sys.stderr) | ||
exit(-1) |
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.
you can do self.env.shell.exec(..., check=True)
and then you don't need to add any extra code for checking the return code
|
||
def run(self, env): | ||
self.env = env | ||
python_path = env.config['variables'].get('python') |
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.
you had good luck using sys.executable
instead here
@atexit.register | ||
def close_mock_server(): | ||
p.terminate() |
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.
neato
Issue #, if available:
Description of changes:
TODO:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.