Skip to content

fix: Fix the log leakages in integration tests#787

Merged
ncloudioj merged 1 commit intomainfrom
disco-3226
Feb 10, 2025
Merged

fix: Fix the log leakages in integration tests#787
ncloudioj merged 1 commit intomainfrom
disco-3226

Conversation

@ncloudioj
Copy link
Copy Markdown
Contributor

@ncloudioj ncloudioj commented Feb 10, 2025

References

JIRA: DISCO-3226

Description

Leaking unexpected logs could cause flaky tests in Merino.

PR Review Checklist

Put an x in the boxes that apply

  • This PR conforms to the Contribution Guidelines
  • The PR title starts with the JIRA issue reference, format example [DISCO-####], and has the same title (if applicable)
  • [load test: (abort|skip|warn)] keywords are applied to the last commit message (if applicable)
  • Documentation has been updated (if applicable)
  • Functional and performance test coverage has been expanded and maintained (if applicable)

@ncloudioj ncloudioj requested a review from a team February 10, 2025 21:19
Copy link
Copy Markdown
Contributor

@misaniwere misaniwere left a comment

Choose a reason for hiding this comment

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

LGTM!

with TestClient(asgi_wrapper) as client:
yield client


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could this be the reason why AMO is also failing. does the failing test also depend on this fixture?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, very likely. I checked all the latest test failures, pretty much all of them were caused by log entry assertion errors. Prior to this patch, 6 seemingly unrelated log entries were spitted out from test_manifest.py. Even so when I only ran this test file alone. Probably, there is perhaps something similar running from curated recommedation tests I guess.

DEBUG    {"Timestamp": 1739221032008496896, "Type": "merino.utils.metrics", "Logger": "merino",  "EnvVersion": "2.0", "Severity": 7, "Pid":      metrics.py:48
                             66200, "Fields": {"taskName": "Task-303", "data": "merino.recommendation.prior.update.timing:17|ms|#application:merino-py,deployment.canary:0", "msg": "sending metrics"},
                             "severity": 100}
DEBUG    {"Timestamp": 1739221032010868992, "Type": "merino.utils.metrics", "Logger": "merino", "EnvVersion": "2.0", "Severity": 7, "Pid":      metrics.py:48
                             66200, "Fields": {"taskName": "Task-303", "data": "merino.recommendation.engagement.update.timing:21|ms|#application:merino-py,deployment.canary:0", "msg": "sending
                             metrics"}, "severity": 100}
INFO     {"Timestamp": 1739221032013295104, "Type": "merino.utils.cron", "Logger": "merino", "EnvVersion": "2.0", "Severity": 6, "Pid": 66200,     cron.py:65
                             "Fields": {"taskName": "Task-314", "duration": 0.026813709002453834, "msg": "Cron: successfully ran task fetch_recommendation_fakespot"}, "severity": 200}
DEBUG    {"Timestamp": 1739221032089231104, "Type": "merino.utils.metrics", "Logger": "merino", "EnvVersion": "2.0", "Severity": 7, "Pid":      metrics.py:48
                             66200, "Fields": {"taskName": "Task-303", "data": "merino.response.status_codes.200:1|c|#application:merino-py,deployment.canary:0", "msg": "sending metrics"}, "severity":
                             100}
DEBUG    {"Timestamp": 1739221032091535104, "Type": "merino.utils.metrics", "Logger": "merino",  "EnvVersion": "2.0", "Severity": 7, "Pid":      metrics.py:48
                             66200, "Fields": {"taskName": "Task-303", "data": "merino.providers.initialize.manifest:57|ms|#application:merino-py,deployment.canary:0", "msg": "sending metrics"},
                             "severity": 100}

@ncloudioj ncloudioj added this pull request to the merge queue Feb 10, 2025
Merged via the queue into main with commit 41303db Feb 10, 2025
4 checks passed
@ncloudioj ncloudioj deleted the disco-3226 branch February 10, 2025 21:43
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.

3 participants