Skip to content

Login acceptance tests, improve login form #6944

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

Merged
merged 27 commits into from
Apr 12, 2025

Conversation

sneridagh
Copy link
Member

(WIP)

@sneridagh sneridagh requested a review from pnicolli April 7, 2025 06:35
@pnicolli
Copy link
Contributor

pnicolli commented Apr 7, 2025

Test is simple and straightforward and I'm ok with it at this time. Looking ahead in the future, I am not a fan of having the tests in a different package from the code, but I'm not a cypress expert, I'm not sure if there's a way that is still simple and allows test code to be colocated with add-on code.

@sneridagh
Copy link
Member Author

@pnicolli I had exactly the same doubt yesterday. We need to talk about it. I want to add a couple of more tests, and probably implement a logout. It's what testing things has, it brings more things to do!

@sneridagh
Copy link
Member Author

@pnicolli ready now. I centralized the Cypress testing in @plone/tooling, then all the calls are made from there.
In the process, I also fixed some things here and there, removed legacy things like the i18n script, removed the deps in there, and other improvements.

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Change log tweaks, a question, and a suggestion

if (
/hydrat/i.test(err.message) ||
/ydrat/i.test(err.message) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an accidental deletion?

Also it looks like a copy-paste from a GitHub issue cypress-io/cypress#27204 (comment). I think we should have an issue in Volto's issue tracker to track it and remove it when it is fixed. That will also allow you to spread the maintenance workload and relieve your brain of being the sole source.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a typo, is fine. Sometimes the error message has a Capital letter (because it's the start of a sentence) and in other occasions, it does not. The error is still present in Cypress 14 unfortunately :(

Anyways, it's a minor thing.

@sneridagh
Copy link
Member Author

@pnicolli going ahead and merging! Please tell me if there's anything that you miss/change!

@sneridagh sneridagh merged commit bf7e20d into seven Apr 12, 2025
28 checks passed
@sneridagh sneridagh deleted the someacceptancetestscurrentroutes branch April 12, 2025 22:09
sneridagh added a commit that referenced this pull request Apr 12, 2025
* seven:
  Login acceptance tests, improve login form (#6944)
  Remove news items that have already been synced with main or released
  [Seven] Added logout route (#6972)
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.

3 participants