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

Integration tests predicate on source version check #347

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

dichn
Copy link
Collaborator

@dichn dichn commented Apr 12, 2022

No description provided.

Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

In my opinion, it's not implemented in a good way:

  • every test is explicitly adding this check. If every test should do the check, then it should be implemented in one place where it works automatically for the tests, rather than requiring some duplicated code between each one.
  • I don't think the tests should skip if the expected version is unset. It should just not do that check.

I earlier suggested doing it using a requests Adapter. This allows you to implement some logic around every request/response, so you could implement it once in one place and then know that every test is using it with no additional effort. Did you try that at all?

It would look something like the below. Note I just typed this directly into the comment here and haven't tested it at all.

@pytest.fixture
def requests_session(src_version):
  session = requests.Session()
  if src_version:
    # we have an expected version, mount an adapter which
    # will check for that version
    session.mount('https://', VersionCheckingAdapter(src_version))
  return session

class VersionCheckingAdapter(requests.adapters.HTTPAdapter):
   def __init__(self, expected_version):
        self.__expected_version = expected_version
        super().__init__()

   def send(self, request, *args, **kwargs):
      # always ask for the version
      request.headers["X-Exodus-Query"] = "1"
      # let superclass send the request
      response = super().send(request, *args, **kwargs)
      # check version in response
      got_version = response.headers["X-Exodus-Version"]
      # or just check the revision part, whatever
      if got_version != self.__expected_version:
          raise AssertionError("Expected to run against version %s but server sent X-Exodus-Version: %s" % (self.__expected_version, got_version))
      return response

def test_something(requests_session):
    # by using requests_session rather than requests.Session(),
    # I'm automatically getting the version check
    requests_session.get(...)

@dichn dichn marked this pull request as draft April 13, 2022 03:04
@dichn dichn force-pushed the master branch 3 times, most recently from 591c365 to f16246b Compare April 18, 2022 05:20
@dichn dichn requested a review from rohanpm April 18, 2022 05:21
@dichn dichn marked this pull request as ready for review April 18, 2022 05:21
@@ -47,6 +53,17 @@ def cdn_test_url(request):
return url


@pytest.fixture
def src_version(request):
Copy link
Member

Choose a reason for hiding this comment

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

Minor: this could do with a comment or doc string. The meaning is not obvious.

elif os.environ.get("CODEBUILD_RESOLVED_SOURCE_VERSION"):
version = os.environ.get("CODEBUILD_RESOLVED_SOURCE_VERSION")
else:
pytest.skip("Test skipped because failed to get lambda version")
Copy link
Member

Choose a reason for hiding this comment

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

It still shouldn't be skipping if the version is unknown.

tests/conftest.py Show resolved Hide resolved
@dichn
Copy link
Collaborator Author

dichn commented Apr 19, 2022

update:

  1. add comments for fixture: src_version
  2. when src_version is unknown, the version check will be skipped

@dichn dichn requested a review from rohanpm April 19, 2022 02:04
@dichn dichn merged commit c935379 into release-engineering:master Apr 19, 2022
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