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 storage-browser offline tests #6206

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ashwinkumar6
Copy link
Member

Description of changes

Add offline e2e tests for action menu items

Description of how you validated changes

Tested in cypress locally

Checklist

  • Have read the Pull Request Guidelines
  • PR description included
  • yarn test passes and tests are updated/added
  • PR title and commit messages follow conventional commit syntax
  • If this change should result in a version bump, changeset added (This can be done after creating the PR.) This does not apply to changes made to docs, e2e, examples, or other private packages.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ashwinkumar6 ashwinkumar6 requested a review from a team as a code owner November 25, 2024 21:40
Copy link

changeset-bot bot commented Nov 25, 2024

⚠️ No Changeset found

Latest commit: a03f8e0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Comment on lines 144 to 146
When I type my "email" with status "CONFIRMED"
Then I type my password
Then I click the "Sign in" button
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this is entirely non-blocking of this PR, but would it fit for there to be step specifically for signing into the storage browser which would just be equivalent to these three steps? Having every test start with 3 - 4 steps to get to the right starting place can really cause these files to get large and less straightforward to parse

Copy link
Member Author

@ashwinkumar6 ashwinkumar6 Nov 25, 2024

Choose a reason for hiding this comment

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

Moved these 3 steps to background (beforeEach) on all the feature files.

        And I type my "email" with status "CONFIRMED"
        And I type my password
        And I click the "Sign in" button

The other option would be to directly define a step "I login into storage-browser" in cypress as code

@@ -38,4 +31,3 @@ Feature: Drag and drop files within Storage Browser
Then I press the "{esc}" key
When I drag and drop a file into the storage browser with file name "test.txt"
Then I see "test.txt"

Copy link
Member

Choose a reason for hiding this comment

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

Should there be an EOF newline?

Comment on lines +5 to +7
And I type my "email" with status "CONFIRMED"
And I type my password
And I click the "Sign in" button
Copy link
Member

@jordanvn jordanvn Nov 26, 2024

Choose a reason for hiding this comment

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

Nit: would it make sense to add the line And I click the first button containing "public"? It's also repeated at the start of every scenario, but I could see it going either way as to whether it could be considered "putting the system in a known state before the user starts interacting with it" as Givens are supposed to be used.

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 took this route, it would apply to the other feature files included in these changes as well

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