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.

Sign up for GitHub

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

Jump to bottom

Add new DNS Record form tests #313

Merged
merged 16 commits into from
Mar 11, 2023
Merged

Add new DNS Record form tests #313

merged 16 commits into from
Mar 11, 2023

Conversation

Eakam1007
Copy link
Contributor

@Eakam1007 Eakam1007 commented Mar 10, 2023 •
edited
Loading

Resolves #278, resolves #296

Since writing the tests relied on setting up the shared auth session, these have been combined into one PR. The following changes have been made:

  • Added auth setup file for e2e tests to login as a user and save session data in a JSON file
  • Updated playwright config to add a setup project which uses the auth setup file. Added this setup project as a dependency to all other projects. This means the setup will run before the tests are run
  • Added above JSON file to .gitignore. This file is generated before the tests are run so it is not necessary to save it
  • Added tests for new DNS Record form. Here the saved session generated by the setup is used for the authenticated tests
  • Updated Mobile Safari config as the tests pass locally but fail on CI, as per comment below

Steps to test

You can check the CI to confirm that the tests pass. Alternatively,

  • Ensure that app is setup according to setup instructions
  • Run npm run docker
  • Run npm run build
  • Run npm run test:e2e:run
  • Check that tests added in this PR pass. Note: Some of the tests for Login will fail locally. This is not related to this PR, and can be investigated in another issue

@Eakam1007 Eakam1007 added this to the Milestone 0.5 milestone Mar 10, 2023
@Eakam1007 Eakam1007 self-assigned this Mar 10, 2023
@Eakam1007
Copy link
Contributor Author

Eakam1007 commented Mar 10, 2023 •
edited
Loading

Hmmm, not quite sure why tests only fail on Mobile Safari in CI. They pass locally

@Eakam1007
Copy link
Contributor Author

CI issue with Mobile Safari seems to be related to this open issue in playwright.
So, using the workaround specified in issue discussion for now: https://www.github.com/microsoft/playwright/issues/11812#issuecomment-1462829766

@Eakam1007 Eakam1007 marked this pull request as ready for review March 10, 2023 11:07
humphd
humphd requested changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is great to see. cc @Myrfion for another reviewer, since he wrote this.

Should we add tests for validation errors on required fields? For example, if we put an IP address for a CNAME? Could do that in follow-ups too.

test/e2e/new-dns-record.spec.ts Outdated Show resolved Hide resolved
test/e2e/new-dns-record.spec.ts
test('does not create dns record if required fields are empty', async ({ page }) => {
await page.goto('/domains/new');
await page.getByRole('button', { name: 'Create' }).click();
await expect(page).toHaveURL(/.*domains\/new/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to check for an error message in the page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't any way to check the error messages since front end validation exists through Chakra UI components:
image
I tried to grab it but I am pretty sure it is not possible

Copy link
Contributor

Choose a reason for hiding this comment

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

In a follow-up, we could style the control and check for that I guess.

test/e2e/new-dns-record.spec.ts Show resolved Hide resolved
@humphd humphd requested a review from Myrfion March 10, 2023 13:20
@Eakam1007
Copy link
Contributor Author

Should we add tests for validation errors on required fields? For example, if we put an IP address for a CNAME? Could do that in follow-ups too.

Hmm, I don't those validations exist yet. I think those can be added after #277 or as part of it

@Eakam1007 Eakam1007 requested review from humphd and Ririio March 11, 2023 20:14
humphd
humphd previously approved these changes Mar 11, 2023
View reviewed changes
playwright.config.ts Show resolved Hide resolved
test/e2e/new-dns-record.spec.ts Outdated Show resolved Hide resolved
@Eakam1007 Eakam1007 requested a review from humphd March 11, 2023 21:42
humphd
humphd approved these changes Mar 11, 2023
View reviewed changes
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Awesome

@Eakam1007 Eakam1007 merged commit ba6e519 into DevelopingSpace:main Mar 11, 2023
@Eakam1007 Eakam1007 deleted the 278-296-add-dns-record-tests branch March 11, 2023 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SerpentBytes SerpentBytes SerpentBytes approved these changes

@sfrunza13 sfrunza13 sfrunza13 approved these changes

@humphd humphd humphd approved these changes

@Myrfion Myrfion Awaiting requested review from Myrfion

@Ririio Ririio Awaiting requested review from Ririio

Assignees

@Eakam1007 Eakam1007

Labels
None yet
Projects
None yet
Milestone
Milestone 0.5
Development

Successfully merging this pull request may close these issues.

Setup shared session for end to end tests Add end to end tests for DNS Record creation form
5 participants
@Eakam1007 @humphd @Ririio @SerpentBytes @sfrunza13