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

Move to using communicate with a timeout for integ tests #1571

Merged
merged 9 commits into from Nov 23, 2019

Conversation

jfuss
Copy link
Contributor

@jfuss jfuss commented Nov 21, 2019

Issue #, if available:

Description of changes:

Checklist:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jfuss jfuss force-pushed the fix/integ-tests-use-communicate branch from 7bd382b to 8cc80cf Compare November 22, 2019 04:06
@jfuss jfuss marked this pull request as ready for review November 23, 2019 15:55
@jfuss jfuss force-pushed the fix/integ-tests-use-communicate branch from 280540f to 476d069 Compare November 23, 2019 15:56
@@ -75,7 +75,7 @@ test_script:
- "venv\\Scripts\\activate"
- "docker system prune -a -f"
- "pytest -vv tests/integration"
- "pytest -vv tests/regression"
- "pytest -n 4 -vv tests/regression"
Copy link
Contributor

Choose a reason for hiding this comment

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

YES!!!!1

@@ -128,11 +118,11 @@ for:
- "pylint --rcfile .pylintrc samcli"
# There are some functional tests that are currently broken due to not being updated with changed code or still running with node4.3 runtimes
# We need to update those but this allows us to at least runs the ones we currently have working
- "pytest tests/functional/commands/validate tests/functional/commands/cli/test_global_config.py"
- "pytest -n 4 tests/functional/commands/validate tests/functional/commands/cli/test_global_config.py"

# Runs only in Linux
- sh: "pytest -vv tests/integration"
Copy link
Contributor

Choose a reason for hiding this comment

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

in the future, we should parallelize this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I tried to do that in this PR but required some additional changes in the build tests. We need to fix this part of the tests: https://github.com/awslabs/aws-sam-cli/blob/develop/tests/integration/buildcmd/test_build_cmd.py#L72 because we validate the image was deleted. I think we did this to ensure new tests would all start with downloading images and from the same place. While I agree, this actually added mins to the test runs as well. I was going to try and run all other integ tests but build but wanted to try and keep things scoped and not solve everything at once here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correction: That method verifies we are deleting the container not the image.

@jfuss jfuss force-pushed the fix/integ-tests-use-communicate branch from 476d069 to c4fd641 Compare November 23, 2019 18:50
@jfuss jfuss merged commit 0431602 into aws:develop Nov 23, 2019
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.

None yet

3 participants