[GPCAPIM-260]-[Steel Thread integration testing]-[RP]#86
[GPCAPIM-260]-[Steel Thread integration testing]-[RP]#86BJSS-russell-pollock wants to merge 2 commits intomainfrom
Conversation
|
|
Deployment Complete
|
|
✅ Trivy gate: no Critical/High issues. Trivy IaC (Terraform) Summary
Findings (top 50)
|
|
✅ Trivy gate: no Critical/High vulnerabilities. Trivy Image Scan SummaryImage: 900119715266.dkr.ecr.eu-west-2.amazonaws.com/whoami:feature-gpcapim-260
Findings (top 50)
|
nhsd-jack-wainwright
left a comment
nhsd-jack-wainwright
left a comment
There was a problem hiding this comment.
Looks good to me 👍, just a few minor points / suggestions
| retention-days: 30 | ||
|
|
||
| - name: Check unit-tests.xml exists | ||
| id: check-unit |
There was a problem hiding this comment.
Would we ever expect this unit-tests.xml file to not exist? Maybe it would be nicer to just let this step fail if the file doesn't exist, as I think this would point to something else going wrong anyway?
| path: gateway-api/test-artefacts/ | ||
| retention-days: 30 | ||
|
|
||
| - name: Check contract-tests.xml exists |
There was a problem hiding this comment.
As above would we ever expect the contract-tests.xml file to not exist here?
| path: gateway-api/test-artefacts/ | ||
| retention-days: 30 | ||
|
|
||
| - name: Check schema-tests.xml exists |
There was a problem hiding this comment.
As above, I think schema-tests.xml should always exist?
| path: gateway-api/test-artefacts/ | ||
| retention-days: 30 | ||
|
|
||
| - name: Check integration-tests.xml exists |
There was a problem hiding this comment.
As above, I think integration-tests.xml should always exist?
| path: gateway-api/test-artefacts/ | ||
| retention-days: 30 | ||
|
|
||
| - name: Check acceptance-tests.xml exists |
There was a problem hiding this comment.
As above, I think acceptance-tests.xml should always exist?
| # Cleanup after tests | ||
| - name: Remove mTLS temp files | ||
| if: github.event.action != 'closed' | ||
| run: rm -f /tmp/client1-key.pem /tmp/client1-cert.pem || true |
There was a problem hiding this comment.
At this point would these temporary files always exist? If so, perhaps it would be better to let this step fail with an unsuccessful exit code rather than providing a default true statement?
| run: rm -f /tmp/client1-key.pem /tmp/client1-cert.pem || true | |
| run: rm -f /tmp/client1-key.pem /tmp/client1-cert.pem |
| headers=headers, | ||
| data=body, | ||
| cert=self.cert, | ||
| verify=False, |
There was a problem hiding this comment.
Given we have the certificate file and key, I think we should be able to verify the certificate provided by the server here?
| # Verify the response matches expectations | ||
| assert response.status_code == 200 | ||
| body = response.json() | ||
| assert body["resourceType"] == "Bundle" |
There was a problem hiding this comment.
Are these assertions being covered in tests elsewhere?
|
|
||
| cert = None | ||
| cert_path = os.getenv("MTLS_CERT") | ||
| key_path = os.getenv("MTLS_KEY") |
There was a problem hiding this comment.
Would it be nicer here if these values were provided as fixtures to the test rather than having the test need to fetch the environment variables itself? The tests would then be able to depend on these values as arguments passed into the methods themselves
3 participants
Description
Move all pipline tests from stage-2 into preview-env
Context
Allows for the tests to be ran with the preview environment
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.