Skip to content

Conversation

@gordonmleigh
Copy link

@gordonmleigh gordonmleigh commented Oct 15, 2019

Issue #, if available: #1434

Description of changes: PR #1242 introduced a bug that means lambdas do not get called for OPTIONS requests, even if configured.

Checklist:

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

@gordonmleigh
Copy link
Author

I'm not entirely sure how to write a test for this, I'm brand new to this project (and python actually). When I run make test I get a bunch of INTERNALERROR lines.

From reading the code, I don't see why this bug that is fixed here wouldn't have failed the TestService_FlaskDefaultOptionsDisabled test.

@jfuss
Copy link
Contributor

jfuss commented Oct 21, 2019

@gordonmleigh

| When I run make test I get a bunch of INTERNALERROR lines.

Are you using a venv? What python version are you installed into (should be python3.7 or greater)?

I don't think I have seen this before. Couple things to do,

  • Rebase with the HEAD of develop (this is probably needed regardless).
  • Clean you venv, and run make init again.

If non-of that works, can you paste in one of the failures, so I can see the stack trace?

| From reading the code, I don't see why this bug that is fixed here wouldn't have failed the TestService_FlaskDefaultOptionsDisabled test.

Most likely because this was a functional tests and we are not currently running any of them in appveyor :(

| I'm not entirely sure how to write a test for this, I'm brand new to this project (and python actually).

This should update any unit tests and add one if needed. The unit tests for this class are found tests/unit/local/apigw/test_local_apigw_service.py.

We should make sure the follow integ tests cases are covered:

  • If there is an cors options method set that points to a Lambda function, the lambda function gets invoked
  • If there is cors options method set with no Lambda function, the Cors behavior wins (which I think is the current behavior).

The integ tests can be found tests/integration/local/start-api/test-start-api.py.

If you need further help, just let me know.

@gordonmleigh
Copy link
Author

@jfuss you can see on the appveyor builds the error I'm talking about. Ends with:

INTERNALERROR>   File "/home/appveyor/venv3.7.4/lib/python3.7/site-packages/pytest_forked/__init__.py", line 41, in pytest_runtest_protocol
INTERNALERROR>     if item.config.getvalue("forked") or item.get_closest_marker("forked"):
INTERNALERROR> AttributeError: 'TestCaseFunction' object has no attribute 'get_closest_marker'

@jfuss
Copy link
Contributor

jfuss commented Oct 22, 2019

@gordonmleigh That is defiantly weird and something I haven't seen before. We are working on making the test suite more reliable. Can you update from HEAD of develop? Hoping some of things just goes away with the recent work we have been doing.

@gordonmleigh
Copy link
Author

@jfuss ah right sorry thought I had updated before getting back to you but was pulling from my own fork. I'll add the tests shortly, thanks for your help.

FYI I think the error is related to the pytest version, I see you've updated to 3.6.0 from 3.1.0 in requirements/dev.txt. The missing functionality was added in 3.6.0 (see pytest-dev/pytest@4914135fd).

@gordonmleigh
Copy link
Author

@jfuss I can't run the integration tests now, due to the following error:

======================================================================== ERRORS ========================================================================
_______________________________________ ERROR collecting tests/integration/local/invoke/test_integrations_cli.py _______________________________________
../../.pyenv/versions/3.7.2/lib/python3.7/unittest/loader.py:235: in getTestCaseNames
    testFnNames = list(filter(shouldIncludeMethod, dir(testCaseClass)))
../../.pyenv/versions/3.7.2/lib/python3.7/unittest/loader.py:232: in shouldIncludeMethod
    fullName = '%s.%s' % (testCaseClass.__module__, testFunc.__qualname__)
E   AttributeError: 'MarkDecorator' object has no attribute '__qualname__'
__________________________________ ERROR collecting tests/integration/local/invoke/runtimes/test_with_runtime_zips.py __________________________________
../../.pyenv/versions/3.7.2/lib/python3.7/unittest/loader.py:235: in getTestCaseNames
    testFnNames = list(filter(shouldIncludeMethod, dir(testCaseClass)))
../../.pyenv/versions/3.7.2/lib/python3.7/unittest/loader.py:232: in shouldIncludeMethod
    fullName = '%s.%s' % (testCaseClass.__module__, testFunc.__qualname__)
E   AttributeError: 'MarkDecorator' object has no attribute '__qualname__'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 2 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
=============================================================== 2 error in 1.03 seconds ================================================================

Same story if I run on develop HEAD. Any ideas?

@jfuss
Copy link
Contributor

jfuss commented Oct 23, 2019

@gordonmleigh My hunch is that its something with pytest and unittest (found some issue last night when helping someone else with this exact issue but can't seem to find it at the moment). What ended up working for her was upgraded from 3.7.2 to 3.7.4. Can you give that a try?

From your recent error, it's comming from the python std lib (unittest).

@jfuss
Copy link
Contributor

jfuss commented Oct 25, 2019

@gordonmleigh Also double check the pytest version that is installed on your system. For whatever reason, when the other person I was helping upgraded pytest to what is was in the dev.txt, things worked. Could be pip acting weird or so PATH issue maybe.

@douglasnaphas
Copy link
Contributor

I pulled the commits from this PR down and opened #1649, with an additional change that gets the integration tests passing while (as far as I can tell from my testing) still fixing the underlying OPTIONS issue #1434.

@jfuss
Copy link
Contributor

jfuss commented Feb 18, 2020

#1649 was approved and was targeting the same bug. Closing this one in favor of the newer one.

@jfuss jfuss closed this Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/local/start-api sam local start-api command type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants