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

Add page object structure for User creation tests #3219

Merged
merged 23 commits into from
Jan 20, 2025

Conversation

vicenteqa
Copy link
Contributor

@vicenteqa vicenteqa commented Jan 8, 2025

Description

  • Encapsulate logic within page objects: each page has a dedicated class containing its selectors, actions, and validations.
  • Tests become more readable and easier to understand by abstracting complexity into reusable methods.
  • Four page objects have been created: a base class for shared functionality, a page object for the initial user listing, and one for the user management flow, and another for the dashboard.
  • Tests are fully decoupled from one another. While this may slightly increase the overall test execution time, it significantly improves reliability and reduces flakiness.

While there are different opinions on using the Page Object pattern in Cypress, I believe it offers a more maintainable approach given the size of our test suite. It allows us to keep app actions and functions organized within dedicated classes, rather than spreading them across the commands.js file or directly in the test files. This structure improves readability, as test files should clearly communicate what our product does without unnecessary noise or distractions.

No changes required in docs.

Fixes # (issue)

Did you add the right label?

Remember to add the right labels to this PR.

  • DONE

How was this tested?

Describe the tests that have been added/changed for this new behavior.

  • DONE

Did you update the documentation?

Remember to ask yourself if your PR requires changes to the following documentation:

Add a documentation PR or write that no changes are required for the documentation.

  • DONE

@vicenteqa vicenteqa marked this pull request as ready for review January 15, 2025 14:47
@vicenteqa vicenteqa removed the wip label Jan 15, 2025
@vicenteqa vicenteqa requested review from janvhs and CDimonaco January 16, 2025 11:00
@balanza
Copy link
Member

balanza commented Jan 16, 2025

I think that's a very valuable approach. Historically, we tend to be explicit in tests and don't use too many helpers or abstractions; nevertheless in this case our previous approach went a bit too far and some benefit was lost, in my opinion.

I want to discuss a bit about the problems page objects are blamed for bringing to a cypress codebase.
My aim is to refine the principles on which we will write tests in the future. Reading from the article you referenced:
TL;DR Points (1) and (4) are not a problem, points (2) and (3) might be but we have solutions

1. Page objects are hard to maintain and take away time from actual application development. I have never seen PageObjects documented well enough to actually help one write tests.

The hard part to maintain is the cypress selectors, and we will have that problem anyway. The worst I see is we just move them to another file.

2. Page objects introduce additional states into the tests, which are separate from the application’s internal state. This makes understanding the tests and failures harder.

This is true and it's very dangerous because it hides knowledge from the test cases and makes them hard to understand. However, you don't have to save any state into the object. If any method has no effect on the others, we still have an explicit interface (beware that this is not 100% true because methods act on the same page via the cy object, but this is how cypress works).

3. Page objects try to fit multiple cases into a uniform interface, falling back to conditional logic - a huge anti-pattern in our opinion.

I don't expect more logic than the one required to access page elements and interact with them. Things like HTTP request intercept, timers, etc should be kept on the test case.
Another thing to be aware of is over-parametrization: every parameter we add to the page object method adds an abstraction of some degree; we should use it wisely so as not to fall into this problem

4. Page objects make tests slow because they force the tests to always go through the application user interface.

I don't know if I get this right. All our tests go through the UI anyway, isn't it?

In the end,
we must be careful with the problems cited in (2) and (3). I think we can enforce best practices using adequate patterns.
For example, we can use static functions instead of object methods to make issue (2) impossible to happen.
If I look at the code, global values are always just constants so we can get rid of this! Without this, we cannot have the problem (2) and we can also limit (3).

I propose the following pattern:

  • each page object is written in the form of a module
  • each object method becomes an exported function of the module
  • each object attribute becomes a module constant (unexported)

Something like this:

// dashboard-po.js

export * from './base-po.js' // if you want to keep inheritance, but it can be removed

const usernameInputField = 'input[autocomplete="username"]';
const passwordInputField = 'input[autocomplete="current-password"]';
...

export function login(username, password) {
    cy.get(usernameInputField).type(username);
    cy.get(passwordInputField).type(password);
    return clickSubmitLoginButton();
  }

export function clickSubmitLoginButton() {
  ...
}

...

What do you think?

@vicenteqa
Copy link
Contributor Author

vicenteqa commented Jan 16, 2025

I'll reply point by point :)

  1. What I was actually seeing is that we had many selectors repeated around the code and with this pattern we remove that problem.
  2. Regarding this point, what is true is that when there was a failure, with the previous approach you knew the exact line that you had to fix, now it requires a bit more of debugging until you know which line on the test is causing the issue, it's a trade off but I don't see it as a big problem (given the things we gain).
  3. Absolutely, we don't need more than what you mention, anyway I see value in hiding as much as possible technicalities (api calls etc) relative to how we test, in the test file itself. The whole point of this pattern/approach is to have clean test files that also serves us as a test repository, with a quick read of the test file you can understand what the test is checking without having to check the details of the functions.

As an example:
with this piece of code it's hard to understand what the test does unless you know javascript

      cy.wait(30000)
        .then(() => TOTP.generate(Cypress.env('totp_secret')))
        .as('otpCode');

      cy.get('@otpCode').then(({ otp }) => {
        cy.get('input[data-testid="login-totp-code"]').type(otp);
      });

with this, without even knowing how to code you get what the test does, the page object gives you context and the function name describes what it does
loginPage.waitForNewTotpCodeAndTypeIt(totpSecret);

  1. I think the author means that using this pattern forces you to interact with the UI instead of using API calls shortcuts to make your tests faster, which is actually not true. You still can put those API calls in whatever page object makes sense (which is what I actually did).

Regarding using the modules structure instead of a class I think we can try it, as in the end, we can achieve the same objective still

The base page object is basically for common stuff (navigation menu) or generic functions that can be custom in every PO to write less code

Last but not least, a big win with this approach is that it helps a lot to maintain the tests isolated from each other. Now, you can run any test on this file independently without the need to execute some others previously, which will make our lives easier in case of a failure.

@balanza
Copy link
Member

balanza commented Jan 16, 2025

  1. I know, that's why I like your proposal
  2. Good point, I didn't think about that. What's the result? Do you think it will be harder to get why a test fails?
  3. Agree

@vicenteqa
Copy link
Contributor Author

  1. I know, that's why I like your proposal
  2. Good point, I didn't think about that. What's the result? Do you think it will be harder to get why a test fails?
  3. Agree
  1. it's just that you will see in which line of the page object is the failure, instead of which line of the test, like any other stack trace, while with the previous approach you could know the exact line of the test that was failing, it is just one more step, but not a big deal imho.

btw, the module structure has been commited & pushed :)

Copy link
Member

@janvhs janvhs left a comment

Choose a reason for hiding this comment

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

Wow that's a huge PR. It's incredibly nice to have this. I honestly didn't read all the code, but as @balanza was pointing out the new module structure is great!

@vicenteqa vicenteqa merged commit 037ddb0 into main Jan 20, 2025
38 checks passed
@vicenteqa vicenteqa deleted the test/poc-pageobjects-users-tests branch January 20, 2025 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants