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.

Sign up for GitHub

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

Jump to bottom

Fix Network Interface Test #605

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Fix Network Interface Test #605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cedc2f4
remove test
waahm7 Oct 18, 2024
eb185da
assert IsNone
waahm7 Oct 18, 2024
af839f0
assert raises
waahm7 Oct 18, 2024
b46e587
comment
waahm7 Oct 18, 2024
11cb281
Merge branch 'main' into fix-test
waahm7 Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crt/aws-c-common
Open in desktop
Submodule aws-c-common updated 2 files
+1 −1 cmake/AwsPrebuildDependency.cmake
+9 −4 include/aws/common/condition_variable.h
2 changes: 1 addition & 1 deletion crt/aws-c-io
Open in desktop
Submodule aws-c-io updated 7 files
+5 −0 include/aws/io/socket.h
+8 −0 source/posix/socket.c
+6 −0 source/windows/iocp/socket.c
+24 −0 source/windows/windows_pki_utils.c
+2 −0 tests/CMakeLists.txt
+14 −0 tests/socket_test.c
+36 −0 tests/tls_handler_test.c
2 changes: 1 addition & 1 deletion crt/aws-c-s3
Open in desktop
Submodule aws-c-s3 updated 18 files
+3 −0 .gitignore
+8 −0 include/aws/s3/private/s3_meta_request_impl.h
+1 −0 include/aws/s3/s3.h
+1 −0 include/aws/s3/s3_client.h
+1 −0 source/s3.c
+55 −33 source/s3_client.c
+25 −1 source/s3_copy_object.c
+16 −4 source/s3_meta_request.c
+5 −0 source/s3_util.c
+5 −0 tests/CMakeLists.txt
+15 −0 tests/mock_s3_server/CreateMultipartUpload/request_timeout.json
+101 −236 tests/s3_data_plane_tests.c
+57 −6 tests/s3_mock_server_tests.c
+44 −0 tests/s3_s3express_client_test.c
+258 −0 tests/s3_tester.c
+27 −0 tests/s3_tester.h
+4 −0 tests/test_helper/README.md
+4 −5 tests/test_helper/test_helper.py
2 changes: 1 addition & 1 deletion crt/aws-lc
Open in desktop
Submodule aws-lc updated from 8b2ebf to 8ffe27
5 changes: 3 additions & 2 deletions test/test_s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,9 @@ def test_sanity_secure(self):
self.assertIsNotNone(s3_client)

def test_sanity_network_interface_names(self):
s3_client = s3_client_new(True, self.region, network_interface_names=("eth0", "eth1"))
self.assertIsNotNone(s3_client)
# This is just a sanity test to ensure that we are passing the parameter correctly.
with self.assertRaises(Exception):
s3_client_new(True, self.region, network_interface_names=("eth0", "invalid-network-interface"))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also check the happy case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not easy to test since only a few platforms have a happy case, and even then, it's different for each platform. This test is only verifying that we pass the value to c-s3 correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add a comment here that the functionality was tested in C properly, and here we make sure the binding works.

Copy link
Contributor Author

@waahm7 waahm7 Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think "sanity" in the test name already signifies this intent. I can add a comment if you still think we should include one here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we have bunch of sanity in the name of the test in aws-c-*, they are meant to simply test it works in the happy case,. eg: https://github.com/awslabs/aws-c-http/blob/main/tests/CMakeLists.txt#L410

I don't think we have a fairly common agreement on "sanity" in the test name means.

It's fine to me. But just mentioning.


def test_wait_shutdown(self):
s3_client = s3_client_new(False, self.region)
Expand Down
Loading