Skip to content
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

Cypress test for service name #1453

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

BenSurgisonGDS
Copy link
Contributor

Cypress test to detect the service name change in the app config

@BenSurgisonGDS BenSurgisonGDS self-assigned this Jul 4, 2022
@BenSurgisonGDS BenSurgisonGDS requested a review from lfdebrux July 4, 2022 14:21
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 4, 2022 14:21 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 4, 2022 14:34 Inactive
@BenSurgisonGDS BenSurgisonGDS force-pushed the integration-test-for-service-name branch from 27e5e0b to 1cdcf79 Compare July 4, 2022 14:34
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 4, 2022 14:34 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 4, 2022 22:36 Inactive
@BenSurgisonGDS BenSurgisonGDS force-pushed the integration-test-for-service-name branch from 8b18a63 to 2015034 Compare July 4, 2022 22:36
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 4, 2022 22:37 Inactive
Comment on lines 95 to 92
try {
const content = fs.readFileSync(filename).toString()
fs.writeFileSync(filename, content.replace(originalText, newText))
return Promise.resolve(null)
} catch (err) {
return Promise.reject(err)
}
Copy link
Member

Choose a reason for hiding this comment

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

If we're writing an async function is there any reason not to use the fs.promises methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this to use fs.promises but had to still return Promise.resolve(null) so that cypress was happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this back to draft while I wait for the PR to use fs.promises is merged into main. I'll then rebase this on to main and amend this test to match.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 5, 2022 14:57 Inactive
@BenSurgisonGDS BenSurgisonGDS requested a review from lfdebrux July 5, 2022 15:13
@BenSurgisonGDS BenSurgisonGDS marked this pull request as draft July 6, 2022 08:10
@BenSurgisonGDS BenSurgisonGDS force-pushed the integration-test-for-service-name branch from 30e26f8 to 382ce38 Compare July 6, 2022 09:37
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 6, 2022 09:37 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 6, 2022 11:31 Inactive
@BenSurgisonGDS BenSurgisonGDS force-pushed the integration-test-for-service-name branch from 750e072 to 83795b7 Compare July 6, 2022 11:32
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 6, 2022 11:32 Inactive
@@ -50,6 +51,8 @@ module.exports = (on, config) => {

const waitUntilAppRestarts = async (timeout) => await waitOn({ delay: 2000, resources: [config.baseUrl], timeout })

const makeSureCypressCanInterpretTheResult = () => null
Copy link
Contributor

Choose a reason for hiding this comment

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

As I commented here https://github.com/alphagov/govuk-prototype-kit/pull/1454/files#r915009116

I think it's worth passing through any non-undefined result unless you're sure it makes no difference.

Also, should this function be centralised in test helpers somewhere?

If I'm overcomplicating it I'm happy with it as-is and we can update if it causes a problem.

@BenSurgisonGDS BenSurgisonGDS force-pushed the integration-test-for-service-name branch from 83795b7 to b9a6b95 Compare July 6, 2022 23:09
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 6, 2022 23:09 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 6, 2022 23:15 Inactive
@BenSurgisonGDS BenSurgisonGDS force-pushed the integration-test-for-service-name branch from 39a81ce to 20e06e1 Compare July 6, 2022 23:16
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-protot-integratio-09yuyg July 6, 2022 23:16 Inactive
@BenSurgisonGDS BenSurgisonGDS marked this pull request as ready for review July 7, 2022 09:42
@BenSurgisonGDS BenSurgisonGDS merged commit e5aaae8 into main Jul 7, 2022
@BenSurgisonGDS BenSurgisonGDS deleted the integration-test-for-service-name branch July 7, 2022 09:42
@BenSurgisonGDS BenSurgisonGDS linked an issue Jul 22, 2022 that may be closed by this pull request
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.

Increase Integration test coverage for prototype kit
4 participants