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

fix: express e2e tests #2708

Merged
merged 18 commits into from
Oct 13, 2022
Merged

fix: express e2e tests #2708

merged 18 commits into from
Oct 13, 2022

Conversation

Benehiko
Copy link
Contributor

@Benehiko Benehiko commented Sep 1, 2022

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I am following the contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security. vulnerability,
    I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added or changed the documentation.

Further Comments

@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #2708 (d8514b5) into master (d8514b5) will not change coverage.
The diff coverage is n/a.

❗ Current head d8514b5 differs from pull request most recent head 26bbb99. Consider uploading reports for the commit 26bbb99 to get more accurate results

@@           Coverage Diff           @@
##           master    #2708   +/-   ##
=======================================
  Coverage   75.81%   75.81%           
=======================================
  Files         302      302           
  Lines       17784    17784           
=======================================
  Hits        13483    13483           
  Misses       3270     3270           
  Partials     1031     1031           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

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

Looks good! Just minor things.

We should also try to get rid of the app === 'express' checks again. Do we have plans to bring the new Managed UI to mobile as well?

test/e2e/cypress/support/index.d.ts Outdated Show resolved Hide resolved
test/e2e/cypress/support/index.d.ts Outdated Show resolved Hide resolved
test/e2e/cypress/support/commands.ts Outdated Show resolved Hide resolved
test/e2e/cypress/support/commands.ts Outdated Show resolved Hide resolved
@Benehiko
Copy link
Contributor Author

Looks good! Just minor things.

We should also try to get rid of the app === 'express' checks again. Do we have plans to bring the new Managed UI to mobile as well?

Yeah it is a bit of a mess in here. That's why I will create a nice e2e testing library inside elements to have a standard across stacks (express, react, react-native etc).

Right now this was added initially to reduce flakes since @aeneasr wanted to make sure the correct app was run in the test. That is also why we add a selector to the application on the body or parent container.

https://github.com/ory/kratos-selfservice-ui-node/pull/189/files#diff-a2e0bd2454eead56ab21f734d4c6ac807de3dd1d6e05fd4e0917125533d07030R15

Copy link
Member

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

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

I think you need to update the ory-prettier-styles package by running npm i in the root. Other than that, LGTM.

jonas-jonas
jonas-jonas previously approved these changes Sep 26, 2022
@Benehiko
Copy link
Contributor Author

Found some failing tests while updating the express app. Will need to fix them first before this can be merged.

@aeneasr aeneasr merged commit 84ea0cf into master Oct 13, 2022
@aeneasr aeneasr deleted the e2e-express branch October 13, 2022 09:19
harnash pushed a commit to Wikia/kratos that referenced this pull request Oct 13, 2022
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
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.

None yet

3 participants