Skip to content

Fix integration tests #48

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

Closed
wants to merge 1 commit into from
Closed

Fix integration tests #48

wants to merge 1 commit into from

Conversation

skwashd
Copy link

@skwashd skwashd commented Sep 4, 2021

Issue #, if available:

Description of changes:
The integration tests were failing due to multiple issues.

Name conflicts

Multiple tests were using the same container name (testing),
which meant some of the tests weren't running. Now each container
has a unique name.

Port conflicts

The same port (9000) was being used for multiple tests. This
wasn't showing as an issue as these tests also used the same
container name. The ports are now unique per test, with them
being number sequentially to avoid any conflicts.

Teardown properly

Only some of the running containers were terminated when the suite
finished executing. This caused many tests to not run properly on
subsequent invocation of the suite. Now all containers are
terminated when the tests complete.

Wrong order of operations

The timeout tests weren't waiitng for the container be ready. This was
caused by the order of operations being wrong, with the sleep step
occurring after the request to the endpoint, not before.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

The integration tests were failing due to multiple issues.

Multiple tests were using the same container name (`testing`),
which meant some of the tests weren't running. Now each container
has a unique name.

The same port (9000) was being used for multiple tests. This
wasn't showing as an issue as these tests also used the same
container name. The ports are now unique per test, with them
being number sequentially to avoid any conflicts.

Only some of the running containers were terminated when the suite
finished executing. This caused many tests to not run properly on
subsequent invocation of the suite. Now all containers are
terminated when the tests complete.

The timeout tests weren't waiting for the container be ready. This was
caused by the order of operations being wrong, with the sleep step
occurring after the request to the endpoint, not before.

This commit fixes aws#47.
@valerena
Copy link
Contributor

valerena commented Oct 4, 2021

Hi @skwashd, thanks for this PR. We took your suggestions into account and fixed the integration tests during our PR last week to support arm64 architecture ( #52 ). Your PR also helped us realize that we had lost the latest tests (that had the sleep in the wrong order and you fixed), and we included your fix when bringing back those tests as well.

So, I'll close this PR since it's no longer relevant, but we'll review the other ones that you sent. Sorry for the delay.

@valerena valerena closed this Oct 4, 2021
@skwashd skwashd deleted the fix-tests branch October 4, 2021 14:12
@skwashd
Copy link
Author

skwashd commented Oct 4, 2021

@valerena thanks for the update. It's good to know that it was useful even if it didn't get committed. Related to this, it would be good to see GitHub Actions or something else running tests so problems like this aren't missed in the future.

@valerena
Copy link
Contributor

valerena commented Oct 4, 2021

@skwashd For sure. We've changed the maintainer team a couple of times, so we're still working on making it better. Thanks for the suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants