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

test[e2e]: add more components to create-flow test #3630

Merged
merged 34 commits into from
Sep 17, 2024

Conversation

jamdelion
Copy link
Contributor

@jamdelion jamdelion commented Sep 5, 2024

In this PR

✅ I think this is the end of the 'straightforward' components, at least from the adding to a flow side.

  • Add tasklist, next steps and review components
  • Add find property, draw boundary, file upload and planning constraints components
  • Refactor to use an enum instead of magic strings!

⚔️ There are other components still to cover, but are difficult or skipped here because:

  • they have a required field which is hard to access from playwright (e.g. content, see question below)
  • I don't understand the component and need to ask someone (e.g. filter, portals)
  • I think they are covered by other tests (but still need to check) - e.g. sections, pay, send

Still to do 📝

  • Implement the remaining components
  • Write a test to cover the 'using this flow' side (i.e. member of public's point of view)

❓ Questions

  • There are a lot of input boxes identified by data-placeholder rather than placeholder which makes it harder to grab them in tests. Is there a way round this/can they be changed from data-placeholder to a normal placeholder attribute?

Copy link

github-actions bot commented Sep 5, 2024

Removed vultr server and associated DNS entries

@jamdelion jamdelion changed the title jh/playwright more components test[e2e]: add more components to create-flow test Sep 5, 2024
@jamdelion jamdelion marked this pull request as ready for review September 5, 2024 16:34
Copy link
Contributor

@DafyddLlyr DafyddLlyr 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 looking great 🙌

I'd recommend trying out the Fixture pattern Playwright uses - this would like remove a lot of the boilerplate of passing around page and locationNode and allow us to better model our pages.

There are a lot of input boxes identified by data-placeholder rather than placeholder which makes it harder to grab them in tests. Is there a way round this/can they be changed from data-placeholder to a normal placeholder attribute?

Great question - I'm actually not sure where data-placeholder is coming from (maybe something internal to MUI?). I don't know of any reason we can't apply a placeholder attribute where needed. FYI - we should be migrating towards labels for accessibility on our inputs over placeholders. Currently the Editor is not audited, just the public interface, but this is one of many known improvements we'd need to implement.

e2e/tests/ui-driven/src/create-flow/create-flow.spec.ts Outdated Show resolved Hide resolved
e2e/tests/ui-driven/src/helpers/addComponent.ts Outdated Show resolved Hide resolved
e2e/tests/ui-driven/src/helpers/addComponent.ts Outdated Show resolved Hide resolved
Comment on lines 5 to 6
cd ../e2e
pnpm dlx lint-staged
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've found that I've needed to run pnpm run lint:fix on every commit from the e2e directory so hopefully this fixes that

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the right solution - this seems to be running e2e linting each time a commit is made in the api directory. There should be a pre-commit file in the e2e directory which does this (clearly it's not working though!)

I think we need to actually install lint-staged in the root (docs) and then we can have a per-project husky working as expected?

@jamdelion
Copy link
Contributor Author

This is looking great 🙌

I'd recommend trying out the Fixture pattern Playwright uses - this would like remove a lot of the boilerplate of passing around page and locationNode and allow us to better model our pages.

Thanks @DafyddLlyr - I'm trying this out in the next PR: https://github.com/theopensystemslab/planx-new/pull/3683/files#diff-1307319267989b018c8a9fe05fcaf699ea2d8459c020391fea5868e32719e8dfR108-R127. So far it only looks like I've moved the complexity around a bit but I'm hoping to find a way to "fixturize" this and make it useful for the other tests in create-flow.spec.ts

@jamdelion jamdelion merged commit 8d42e6f into main Sep 17, 2024
12 checks passed
@jamdelion jamdelion deleted the jh/playwright-more-components branch October 28, 2024 16:43
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.

2 participants