-
Notifications
You must be signed in to change notification settings - Fork 3
VED-770-e2e-batch-rewrite #849
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
Conversation
|
This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket: VED-770 |
51dca76 to
3411691
Compare
dlzhry2nhs
left a comment
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.
Nice work - looks like it's running quickly in the pipeline and it'll be great to get this back in as a CI check.
Couple of comments added mostly around readability and expressing intent.
We can also have tickets down the line to add further test cases of critical functionality e.g. invalid files, duplicates, and richer assertions on the persisted data. Happy for that to be later rather than now - this PR gets us to a better position.
Only reservation relates to the readability - while it is now performing, not convinced this is much more readable or extensible than what we had before. We can see when we attempt to add the additional required cases. If necessary could spike moving to pytest parallel - it has always been my preference regardless of whether it is integration or unit tests, for test readability i.e. clear setup, act and assert steps to be used. (Details of what we are doing in the steps can be abstracted into code but in this suite it seems like everything is tucked away into the new test classes away from the original e2e_batch test file.
65eee14 to
436c1b9
Compare
6013e42 to
d4b1183
Compare
ec52c23 to
73357d3
Compare
44fae14 to
e66ad1b
Compare
|
| print(".", end="") | ||
| time.sleep(5) | ||
| if (time.time() - start_time) > max_timeout: | ||
| return False |
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.
just a minor point: if we do timeout, should we not log something? (e.g. the fact we timed out, and/or some information on the unprocessed files)
* Purge SQS in development PR branch only * comments env & create_with_cp1252_encoded_character * poetry lock * remove VaxSupplierPerms. 600 s timeout. readme
* Purge SQS in development PR branch only * comments env & create_with_cp1252_encoded_character * poetry lock * remove VaxSupplierPerms. 600 s timeout. readme



Summary
= Complete cleanup of source/dest s3, audit/events table, SQS FIFOs (only purged for dev PRs)
Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.