-
Notifications
You must be signed in to change notification settings - Fork 24
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
[ BB2-537 ] Create Middleware-Logging Integration Tests #946
[ BB2-537 ] Create Middleware-Logging Integration Tests #946
Conversation
…ate-Middleware-Logging-Tests-IT
…log integration tests.
…ate-Middleware-Logging-Tests-IT
…ate-Middleware-Logging-Tests-IT
…ate-Middleware-Logging-Tests-IT
…ate-Middleware-Logging-Tests-IT
…ate-Middleware-Logging-Tests-IT
…ate-Middleware-Logging-Tests-IT
…ate-Middleware-Logging-Tests-IT
@JFU-GIT Do you think this one will have a lot of changes after 724 is merged in? |
…ate-Middleware-Logging-Tests-IT
…ate-Middleware-Logging-Tests-IT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JFU-GIT This is looking good. Only had one change suggestion, if it looks good to do.
I wasn't able to get it working locally. Having the request time-out issue I was having with BB2-545. Assuming I have some kind of issue in my local environment, since this is working for you locally. And also since I can't get the BB2-545 branch to work currently, but it did in the past. I'll work to troubleshoot later, but will feel good if another reviewer can also run this locally without the errors.
…e to facilitate reviewing and verifying.
…ate-Middleware-Logging-Tests-IT
…ate-Middleware-Logging-Tests-IT
This pull request introduces 2 alerts when merging 4088166 into bf17c5b - view on LGTM.com new alerts:
|
…ate-Middleware-Logging-Tests-IT
This pull request introduces 2 alerts when merging a8d6580 into a05bb44 - view on LGTM.com new alerts:
|
…ate-Middleware-Logging-Tests-IT
This pull request introduces 2 alerts when merging ac6e97b into 2d90d4d - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging b16dd8e into 2d90d4d - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging a1607cf into 2d90d4d - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 1b14c71 into 2d90d4d - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 3c95cd9 into 2d90d4d - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JFU-GIT Just a couple changes to suppress the LGTM alerts. Assuming OK since this is for integration tests.
@@ -0,0 +1,36 @@ | |||
from .dev import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JFU-GIT Same here to suppress LGTM alert:
from .dev import * | |
from .dev import * # lgtm [py/polluting-import] | |
This pull request introduces 1 alert when merging f045a7b into 2d90d4d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 64c1503 into 2d90d4d - view on LGTM.com new alerts:
|
…ate-Middleware-Logging-Tests-IT
This pull request introduces 1 alert when merging d19564b into 82cce07 - view on LGTM.com new alerts:
|
- Add healthcheck to postgres startup and bb2slsx to depend on service_healthy to fix timing issue in docker-compose.selenium.yml - Move DJANGO_SETTINGS_MODULE location in launcher script docker-compose/run_selenium_tests_local_keybase.sh to enable other types of tests (non logit) to run. BB2-537
@dtisza1 I'm able to get this working after your change 👍 |
…ate-Middleware-Logging-Tests-IT
This pull request introduces 1 alert when merging 10f1c99 into b2c2ab8 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JIRA Ticket:
BB2-537
User Story or Bug Summary:
Regression tests should be added for critical request/response middleware logging events.
The request_logging.py middleware is not integrated in to the Django tests. So these can not be tested currently using the same method as the audit type logging.
A/C:
Perform discovery work to determine the best method for validating the logging.
Test coverage of middleware logging (and other specialty logging such as auth flow logging) can be best achieved by validating the real log records collected from close to real life end to end BB2 flows : auth flow and data flow.
In this ticket an integration test approach is adopted to maximize the coverage of various logging components - including log records from logging middleware.
JSONSCHEMA is leveraged in validation logic.
External dependencies:
BB2-545
this PR depends on selenium tests PR
What Does This PR Do?
the implementation leverage selenium tests and bb2 built in testclient to generate a close to real life auth flow + fhir flows, where extensive set of logging middleware log records are generated and these log records are in turn scanned and validated with the extensive set of event json schema.
What Should Reviewers Watch For?
If you're reviewing this PR, please check these things, in particular:
The code and scripts introduced can be further used to test other logging areas e.g. auth flow integration tests.
BB2-723
What Security Implications Does This PR Have?
Submitters should complete the following questionnaire:
What Needs to Be Merged and Deployed Before this PR?
This PR cannot be either merged or deployed until the following pre-requisite changes have been fully deployed:
Submitter Checklist
I have gone through and verified that...:
README
updates and changelog / release notes entries.TODO
and/orFIXME
comments, which include a JIRA ticket ID for any items that require urgent attention.