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

fix: options endpoint does not invoke lambda (fixing CI issues with author's PR) (#1434) #1649

Merged
merged 8 commits into from
Feb 19, 2020

Conversation

douglasnaphas
Copy link
Contributor

@douglasnaphas douglasnaphas commented Dec 9, 2019

Issue #, if available: #1434

Description of changes: This attempts to fix the CI failures encountered with @gordonmleigh's PR #1464.

Checklist:

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

@sanathkr
Copy link
Contributor

sanathkr commented Dec 9, 2019

I am pulling this PR and the original PR down locally to resubmit as one.

@douglasnaphas
Copy link
Contributor Author

I am pulling this PR and the original PR down locally to resubmit as one.

@sanathkr: Do you know what needs to be done to fix the failing checks on this PR and #1464?

@jfuss
Copy link
Contributor

jfuss commented Dec 11, 2019

@douglasnaphas I would start by looking at the failing integration tests.

@douglasnaphas
Copy link
Contributor Author

@sanathkr Did you have any luck resubmitting this along with the original PR?

@douglasnaphas
Copy link
Contributor Author

douglasnaphas commented Jan 27, 2020

Failing tests

  1. tests/integration/local/start_api/test_start_api.py::TestServiceCorsSwaggerRequests::test_cors_swagger_options (1277)
  2. tests/integration/local/start_api/test_start_api.py::TestServiceCorsGlobalRequests::test_cors_global (1281)

Update: These are passing now.

Gordon Leigh and others added 3 commits February 6, 2020 14:00
aws#1434

This is intended to get the following integration tests passing:

tests/integration/local/start_api/test_start_api.py
  TestServiceCorsSwaggerRequests
  TestServiceCorsGlobalRequests

Those tests use a
[CorsConfiguration](https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-property-api-corsconfiguration.html)
and a [Cors
Global](https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-specification-template-anatomy-globals.html),
respectively, to enable CORS.

When CORS is enabled, OPTIONS requests should send a 200 with the
specified CORS headers, even if no OPTIONS handler is explicitly stated
in the pre-transformation SAM template. I confirmed this by deploying
from a [SAM
template](https://github.com/douglasnaphas/play-sam/tree/master/cors-swagger)
similar to one of the failing integration tests (with Cors specified but
with no explicit OPTIONS handler), and observing that it does indeed
respond with a 200 and the CORS headers attached.
aws#1434

This adds an integration test to verify that templates with explicit
OPTIONS handlers actually have those handlers invoked on OPTIONS
requests. There has been an issue with OPTIONS requests returning 200
and not reaching an explicitly specified OPTIONS handler.
aws#1649 fixes that, so if the
integration test added in this commit passes (and fails on the develop
branch), it could be added to that PR.
@douglasnaphas douglasnaphas mentioned this pull request Feb 18, 2020
6 tasks
douglasnaphas added a commit to douglasnaphas/aws-sam-cli that referenced this pull request Feb 18, 2020
aws#1434

This adds an integration test to verify that templates with explicit
OPTIONS handlers actually have those handlers invoked on OPTIONS
requests. There has been an issue with OPTIONS requests returning 200
and not reaching an explicitly specified OPTIONS handler.
aws#1649 fixes that, so if the
integration test added in this commit passes (and fails on the develop
branch), it could be added to that PR.
@douglasnaphas
Copy link
Contributor Author

The integration test added in this PR passes when run locally with SAM_CLI_DEV=1 pytest tests/integration/local/start_api/test_start_api.py -k TestOptionsHandler with this PR's branch checked out.

When I apply the integration test, but not the code change, to the develop branch in this repo, the test fails. This suggests the test is valid.

$ git remote get-url upstream
git@github.com:awslabs/aws-sam-cli.git

$ git fetch upstream

$ git log upstream/develop..HEAD
commit d8a196980bd6f628b18a430598cac722d207e739 (HEAD -> debug-1434-options-itest-fails, origin/debug-1434-options-itest-fails, develop)
Author: Jacob Fuss <jfuss@users.noreply.github.com>
Date:   Tue Feb 18 13:11:58 2020 -0600

    Port to python

commit 57b9a9f7011b608c4190a53c71b189fc676c828c
Author: Douglas Naphas <douglasnaphas@gmail.com>
Date:   Tue Feb 18 13:28:30 2020 -0500

    Add an integration test on OPTIONS handling

    https://github.com/awslabs/aws-sam-cli/issues/1434

    This adds an integration test to verify that templates with explicit
    OPTIONS handlers actually have those handlers invoked on OPTIONS
    requests. There has been an issue with OPTIONS requests returning 200
    and not reaching an explicitly specified OPTIONS handler.                                                                                                                                                                                                                       https://github.com/awslabs/aws-sam-cli/pull/1649 fixes that, so if the
    integration test added in this commit passes (and fails on the develop
    branch), it could be added to that PR.

$ SAM_CLI_DEV=1 pytest tests/integration/local/start_api/test_start_api.py -k TestOptionsHandler
============================================================================================================================= test session starts ==============================================================================================================================
platform darwin -- Python 3.7.4, pytest-5.2.1, py-1.8.0, pluggy-0.13.1
rootdir: /Users/dougnaphas/repos/aws-sam-cli, inifile: pytest.ini
plugins: xdist-1.30.0, timeout-1.3.3, cov-2.7.1, rerunfailures-7.0, forked-1.1.3
collected 82 items / 81 deselected / 1 selected

tests/integration/local/start_api/test_start_api.py R                                                                                                                                                                                                                    [100%]R
 [100%]R [100%]F [100%]

=================================================================================================================================== FAILURES ===================================================================================================================================
___________________________________________________________________________________________________________________ TestOptionsHandler.test_options_handler ____________________________________________________________________________________________________________________

self = <tests.integration.local.start_api.test_start_api.TestOptionsHandler testMethod=test_options_handler>

    @pytest.mark.flaky(reruns=3)
    @pytest.mark.timeout(timeout=600, method="thread")
    def test_options_handler(self):
        """
        This tests that a template's OPTIONS handler is invoked
        """
        response = requests.options(self.url + "/optionshandler", timeout=300)

>       self.assertEqual(response.status_code, 204)
E       AssertionError: 200 != 204

tests/integration/local/start_api/test_start_api.py:822: AssertionError
---------------------------------------------------------------------------------------------------------------------------- Captured stderr setup -----------------------------------------------------------------------------------------------------------------------------
Mounting AOptionsHandlerFunction at http://127.0.0.1:31093/optionshandler [OPTIONS]
You can now browse to the above endpoints to invoke your functions. You do not need to restart/reload SAM CLI while working on your functions, changes will be reflected instantly/automatically. You only need to restart SAM CLI if you update your AWS SAM template
2020-02-18 14:48:45  * Running on http://127.0.0.1:31093/ (Press CTRL+C to quit)
----------------------------------------------------------------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------------------------------------------------------------
2020-02-18 14:48:49 127.0.0.1 - - [18/Feb/2020 14:48:49] "OPTIONS /optionshandler HTTP/1.1" 200 -
---------------------------------------------------------------------------------------------------------------------------- Captured stderr setup -----------------------------------------------------------------------------------------------------------------------------
Mounting AOptionsHandlerFunction at http://127.0.0.1:39064/optionshandler [OPTIONS]
You can now browse to the above endpoints to invoke your functions. You do not need to restart/reload SAM CLI while working on your functions, changes will be reflected instantly/automatically. You only need to restart SAM CLI if you update your AWS SAM template
2020-02-18 14:48:50  * Running on http://127.0.0.1:39064/ (Press CTRL+C to quit)
----------------------------------------------------------------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------------------------------------------------------------
2020-02-18 14:48:54 127.0.0.1 - - [18/Feb/2020 14:48:54] "OPTIONS /optionshandler HTTP/1.1" 200 -
---------------------------------------------------------------------------------------------------------------------------- Captured stderr setup -----------------------------------------------------------------------------------------------------------------------------
Mounting AOptionsHandlerFunction at http://127.0.0.1:34400/optionshandler [OPTIONS]
You can now browse to the above endpoints to invoke your functions. You do not need to restart/reload SAM CLI while working on your functions, changes will be reflected instantly/automatically. You only need to restart SAM CLI if you update your AWS SAM template
2020-02-18 14:48:55  * Running on http://127.0.0.1:34400/ (Press CTRL+C to quit)
----------------------------------------------------------------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------------------------------------------------------------
2020-02-18 14:48:59 127.0.0.1 - - [18/Feb/2020 14:48:59] "OPTIONS /optionshandler HTTP/1.1" 200 -
---------------------------------------------------------------------------------------------------------------------------- Captured stderr setup -----------------------------------------------------------------------------------------------------------------------------
Mounting AOptionsHandlerFunction at http://127.0.0.1:35932/optionshandler [OPTIONS]
You can now browse to the above endpoints to invoke your functions. You do not need to restart/reload SAM CLI while working on your functions, changes will be reflected instantly/automatically. You only need to restart SAM CLI if you update your AWS SAM template
2020-02-18 14:49:00  * Running on http://127.0.0.1:35932/ (Press CTRL+C to quit)
----------------------------------------------------------------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------------------------------------------------------------
2020-02-18 14:49:04 127.0.0.1 - - [18/Feb/2020 14:49:04] "OPTIONS /optionshandler HTTP/1.1" 200 -
================================================================================================================== 1 failed, 81 deselected, 3 rerun in 20.58s ==================================================================================================================

Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for keeping up with this and spending the time to address the bug.

jfuss and others added 3 commits February 18, 2020 14:47
`black --diff tests/integration/local/start_api/test_start_api.py`
reported this issue--a missing newline between classes.
@jfuss jfuss merged commit 526f8da into aws:develop Feb 19, 2020
@douglasnaphas douglasnaphas deleted the debug-1434-options branch February 19, 2020 17:17
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