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

chore(tests): refactor E2E logger to ease maintenance, writing tests and parallelization #1460

Merged

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Aug 18, 2022

Issue number: #1441

Summary

This PR refactors Logger E2E test following the new Infrastructure and explicit tests.

Tasks

  • Create LoggerStack and use new infrastructure
  • get_logs: consistency with other fetcher UX
  • get_logs: accept date time (consistency) and convert to epoch int
  • get_logs: remove lru_cache due to new dynamic nature
  • get_logs: accept filter_expression to reduce unwanted logs
  • get_logs: set filter_expression default to standard Logger key to remove start/end/report log events
  • get_logs: arbitrary additional keys should be added to Log model
  • Test for cold start side effect and cleanup to what only E2E can test
  • Revisit get_logs on whether we could make assertion easier
  • Revisit any Lambda specific feature in Logger we can't test w/ functional tests

Changes

Please provide a summary of what's being changed

User experience

Please share what the user experience looks like before and after this change

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@heitorlessa heitorlessa requested a review from a team as a code owner August 18, 2022 15:42
@heitorlessa heitorlessa requested review from mploski and removed request for a team August 18, 2022 15:42
@boring-cyborg boring-cyborg bot added the tests label Aug 18, 2022
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 18, 2022
@heitorlessa heitorlessa marked this pull request as draft August 18, 2022 15:43
@heitorlessa heitorlessa linked an issue Aug 19, 2022 that may be closed by this pull request
@boring-cyborg boring-cyborg bot added the dependencies Pull requests that update a dependency file label Aug 19, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2022

Codecov Report

Merging #1460 (6981a78) into develop (4d0f1a2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1460   +/-   ##
========================================
  Coverage    99.89%   99.89%           
========================================
  Files          123      123           
  Lines         5496     5497    +1     
  Branches       629      629           
========================================
+ Hits          5490     5491    +1     
  Misses           2        2           
  Partials         4        4           
Impacted Files Coverage Δ
aws_lambda_powertools/shared/constants.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@heitorlessa heitorlessa marked this pull request as ready for review August 19, 2022 13:35
@github-actions github-actions bot added the internal Maintenance changes label Aug 19, 2022
@heitorlessa
Copy link
Contributor Author

Similar to Tracer, I've created a LogFetcher to ease assertion and potential enhancements like assert "key|value|keys" in logs.

Before

def test_basic_lambda_contextual_data_logged(execute_lambda: conftest.InfrastructureOutput):
    # GIVEN
    required_keys = (
        "xray_trace_id",
        "function_request_id",
        "function_arn",
        "function_memory_size",
        "function_name",
        "cold_start",
    )

    lambda_name = execute_lambda.get_lambda_function_name(cf_output_name="basichandlerarn")
    timestamp = execute_lambda.get_lambda_execution_time_timestamp()
    cw_client = boto3.client("logs")

    # WHEN
    filtered_logs = data_fetcher.get_logs(lambda_function_name=lambda_name, start_time=timestamp, log_client=cw_client)

    # THEN
    assert all(keys in logs.dict(exclude_unset=True) for logs in filtered_logs for keys in required_keys)

After

It combines all other tests into a single test by making the payload dynamic.

def test_basic_lambda_logs_visible(basic_handler_fn, basic_handler_fn_arn):
    # GIVEN
    message = "logs should be visible with default settings"
    custom_key = "order_id"
    additional_keys = {custom_key: f"{uuid4()}"}
    payload = json.dumps({"message": message, "append_keys": additional_keys})

    # WHEN
    _, execution_time = data_fetcher.get_lambda_response(lambda_arn=basic_handler_fn_arn, payload=payload)
    data_fetcher.get_lambda_response(lambda_arn=basic_handler_fn_arn, payload=payload)

    # THEN
    logs = data_fetcher.get_logs(function_name=basic_handler_fn, start_time=execution_time)

    assert len(logs) == 2
    assert len(logs.get_cold_start_log()) == 1
    assert len(logs.get_log(key=custom_key)) == 2
    assert logs.have_keys(*LOGGER_LAMBDA_CONTEXT_KEYS) is True

@heitorlessa
Copy link
Contributor Author

@rubenfonseca @mploski @leandrodamascena could either of you have a pass? It's a lot smaller now that we don't need non-related cleanups

@heitorlessa heitorlessa merged commit 6930b42 into aws-powertools:develop Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file internal Maintenance changes size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor E2E: Refactor Logger test and increase coverage
3 participants