FS-5030: Implementing e2e test running after deploying to dev and test environments #328
Conversation
nuwan-samarasinghe
force-pushed
the
FS-5030_e2e_run_env
branch
from
c040e39 to
485af94
Compare
|
Thank you kindly @nuwan-samarasinghe, quick note before I review properly: I notice we're using the standard boilerplate for the PR description here, but in this case it doesn't really apply (no unit tests or documentation updates, and the commit message format differs from our guidelines). The actual description ("Implementing e2e test running after deploying to dev and test environments") gives me a general idea of what's going on, but it'd be nice to see a bit more detail about what you've done and why. PR descriptions are an opportunity to help reviewers understand the changes quickly and provide meaningful feedback. Don't worry about changing the description on this PR, but for future PRs it'd be good to see a bit more information in the description, even if that means ignoring the boilerplate. |
nuwan-samarasinghe
force-pushed
the
FS-5030_e2e_run_env
branch
from
485af94 to
61403fa
Compare
nuwan-samarasinghe
force-pushed
the
FS-5030_e2e_run_env
branch
from
61403fa to
2e65132
Compare
|
There was a problem hiding this comment.
Hey @nuwan-samarasinghe, have just finished reviewing the PR, I have flagged some definite issues and also asked several questions. A more detailed PR description would probably have saved me some confusion 😸
| # environment: dev | ||
| dev_e2e_test: | ||
| # Do not run these against the prod environment without addressing the auth/JWT self-signing done by e2e tests. | ||
| if: ${{ contains(fromJSON(needs.setup.outputs.jobs_to_run), 'dev') && inputs.run_e2e_tests == true }} |
There was a problem hiding this comment.
Not sure this will work - run_e2e_tests is only set as an input param during workflow dispatch (manual trigger via "Actions" tab in GitHub UI), and I believe will default to null in a scenario where a PR is merged. Thus inputs.run_e2e_tests == true will evaluate to false and this job won't run 😿
I think we need something more like the funding-service-pre-award deployment workflow, whereby we have this sort of logic: (github.event_name == 'push' && true) || inputs.run_e2e_tests_assessment - this guarantees the E2E tests run on merge, which is what we're looking for
| # Do not run these against the prod environment without addressing the auth/JWT self-signing done by e2e tests. | ||
| if: ${{ contains(fromJSON(needs.setup.outputs.jobs_to_run), 'test') && inputs.run_e2e_tests == true }} | ||
| name: Run E2E Test | ||
| needs: [ dev_deploy ] |
There was a problem hiding this comment.
This should depend on test_deploy not dev_deploy
| workflow_call: | ||
| inputs: | ||
| copilot_environment: | ||
| description: "Copilot environment to deploy to" |
There was a problem hiding this comment.
Not the best description - this workflow doesn't deploy, it runs E2E tests
| on: | ||
| workflow_call: | ||
| inputs: | ||
| copilot_environment: |
There was a problem hiding this comment.
Prefer environment to copilot_environment as more user-friendly and consistent with other jobs in FAB workflow
| run: uv run --frozen pytest --e2e --e2e-env ${{ inputs.copilot_environment }} | ||
| env: | ||
| E2E_DEVTEST_BASIC_AUTH_USERNAME: ${{ secrets.E2E_DEVTEST_BASIC_AUTH_USERNAME }} | ||
| E2E_DEVTEST_BASIC_AUTH_PASSWORD: ${{ secrets.E2E_DEVTEST_BASIC_AUTH_USERNAME }} |
There was a problem hiding this comment.
Should presumably be secrets.E2E_DEVTEST_BASIC_AUTH_PASSWORD?
Also, how are these env variables actually used?
Given the requirement to do setup external to this PR to get the functionality working, it is extra important to include this info in the PR description to make it clear what steps we need to take alongside simply merging the PR
| environment: ${{ inputs.copilot_environment }} | ||
| steps: | ||
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 | ||
| - name: Install the latest version of uv |
There was a problem hiding this comment.
This is misleadingly named as it's not installing the "latest" version, just the one defined by the specific commit hash
| runs-on: ubuntu-latest | ||
| environment: ${{ inputs.copilot_environment }} | ||
| steps: | ||
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 |
| uses: astral-sh/setup-uv@f94ec6bedd8674c4426838e6b50417d36b6ab231 # v5 | ||
| with: | ||
| enable-cache: true | ||
| github-token: ${{ secrets.CUSTOM_GITHUB_TOKEN }} |
There was a problem hiding this comment.
Do we need this parameter? Just from looking at uv installation in run-shared-tests.yml - https://github.com/communitiesuk/funding-service-design-workflows/blob/main/.github/workflows/run-shared-tests.yml#L168-L169 - there is no github-token parameter passed
|
Ah yeah, separate thing - please let's cache Playwright browsers - downloading them afresh every time we run E2E tests is heavy stuff. Check this out for reference: https://playwrightsolutions.com/playwright-github-action-to-cache-the-browser-binaries/ |
Ticket: https://mhclgdigital.atlassian.net/browse/FS-5030
Change description
Implementing e2e test running after deploying to dev and test environments
After having the discussion with @wjrm500 we have decided to brake this FS-5030 into two sub sections and it is already mentioned in this comment
Once changes are merged into main and docker images is deployed to dev then after that if the e2e run is enabled then this changes will handle the e2e run process against the dev environment & also same for the test once changes are deployed to test it will run the e2e against test environment
Following is one of the testing done by my self : https://github.com/communitiesuk/funding-service-design-fund-application-builder/actions/runs/13763808399