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(quantic): first setup of playwright added in Quantic #4597

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

mmitiche
Copy link
Contributor

@mmitiche mmitiche commented Oct 25, 2024

SFINT-5768

Intro

In this PR, we introduce the Playwright setup for the Quantic project, allowing for automated E2E tests on Lightning Web Components (LWC) in Salesforce. The Playwright structure here includes:

Page Objects:

Page objects encapsulate component selectors and interactions, making tests more modular and maintainable, in the proposed structure we have common page objects and unique page objects.

The common page objects will be shared across all the Quantic E2E tests, for example:

  • ConfigurationObject: responsible of configuring the Quantic Example page to initializes the test environment for a given test.
  • SearchObject: responsible of performing a search and intercepting the search requests in both the search use case and the insight use case.
  • InsightObject: responsible of performing the logic that is specific to the insight use case, like for example waiting the initialization of the Quantic Insight Interface.

A unique page object is an object that is specific to a given component, like in this example the PagerObject

Fixtures:

A Playwright fixture is a reusable setup that provides a consistent environment for tests.

I created a base fixture called quanticBase that will be the base of all the fixture we will create for each Quantic component, this base fixture by default uses ConfigurationObject, SearchObject and InsightObject page objects as these are always needed in any test.

Additionally each component will have his own fixtures that will extend quanticBase, like for example in this example, the QuanticPager has two fixtures testSearch and testInsight, two fixtures that provides the setup to test the component in the search use case and the insight use case respectively.

Tests:

Following this structure, each Quantic component will now contain a new folder called e2e that will contain the page object of the component, the fixtures needed to test and the test spec.
Each component will have the following structure:

QuanticPager/
│
├── e2e/
│   ├── page-object.ts
│   ├── fixture.ts
│   └── quantic-pager.e2e.ts
├── __test__/
│   └── quantic-pager.unit.test.ts
│
├── QuanticPager.js
├── QuanticPager.html
├── QuanticPager.css

Each component will have the component logic, the unit tests and the E2E in the same folder.

Jest Unit tests should focus on the following points:

  • Test component behaviour in isolation.
  • Mock External Dependencies like the capabilities used from the Headless library.
  • Ensure the component renders the correct HTML based on its state and input properties.
  • Verify that component properties update correctly based on user interactions or lifecycle events.
  • Cover edge cases.

Playwright E2E tests should focus on the following points:

  • Test the essential paths that users take to achieve their goals using a given component.
  • Mimic actual user actions, such as clicking buttons, entering data and navigation.
  • Test interactions with external systems like our Coveo APIs and analytics.

Copy link

github-actions bot commented Oct 25, 2024

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 236.8 236.8 0
commerce 341.5 341.5 0
search 412.8 412.8 0
insight 402.1 402.1 0
recommendation 249.1 249.1 0
ssr 406.3 406.3 0
ssr-commerce 353.7 353.7 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

Copy link
Contributor

@SimonMilord SimonMilord left a comment

Choose a reason for hiding this comment

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

I don't dislike having the e2e tests in each component's folder for quantic, I think it makes sense. I really like how readable the tests are using the page-object files.

I have a few questions however, mostly about the tests setup. Since its still in draft I don't know if its the final form or a WIP so please feel free to let me know.

  • Do each component really need their own fixture file? I think we could abstract the setup by making generic testSearch and testInsight fixtures for example and passing the component's fixtures type and their page object as props no? Unless im missing something it would simplify the setup for next tests we add and avoid duplicating code.

  • In e2e test files, in relation to my first point, I think it would be nicer to have functions such as visitPage and setupWithPauseBeforeSearch etc. be abstracted in their own general function that take props. It would reduce complexity in each tests and make it easier to setup no?

@mmitiche
Copy link
Contributor Author

I don't dislike having the e2e tests in each component's folder for quantic, I think it makes sense. I really like how readable the tests are using the page-object files.

I have a few questions however, mostly about the tests setup. Since its still in draft I don't know if its the final form or a WIP so please feel free to let me know.

  • Do each component really need their own fixture file? I think we could abstract the setup by making generic testSearch and testInsight fixtures for example and passing the component's fixtures type and their page object as props no? Unless im missing something it would simplify the setup for next tests we add and avoid duplicating code.
  • In e2e test files, in relation to my first point, I think it would be nicer to have functions such as visitPage and setupWithPauseBeforeSearch etc. be abstracted in their own general function that take props. It would reduce complexity in each tests and make it easier to setup no?

This not WIP, this is my final proposal to get your first feedbacks, additionally this is the first base of the structure that will build upon, which means that of course there will be some tweaks and refactoring when needed we while add all the Quantic E2E tests.
for example if FunctionA is used somewhere and when we add more tests we found that we are re-using it a lot of course of will refactor this function to be written just one time in a general way so it can be imported and used easily in multiple way, but again this will come as our structure grow.

  • For your first question, yes each component needs a fixture file, cause each component has its specific page object and the component needs a fixture that uses this page object.
  • Like I mentioned, this for me is the kind of things that could come or not with time as the structure grows, true that every test start with visiting the page, but from our previous cypress tests experience, the initialization of each component may differ as each component for example needs to mock/listen/modify different part of what's returned in the Search request but this a good point that we should keep an eye on, if we can it would be very nice indeed to improve how initialize our setup environment.

baseURL: process.env.BASE_URL,

/* Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer */
trace: 'on-first-retry',
Copy link
Contributor

Choose a reason for hiding this comment

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

In Atomic we use retain-on-failure.

packages/quantic/playwright.config.ts Outdated Show resolved Hide resolved
packages/quantic/playwright.config.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alexprudhomme alexprudhomme left a comment

Choose a reason for hiding this comment

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

Looks clean to me ! Nice !

Copy link
Contributor

@SimonMilord SimonMilord left a comment

Choose a reason for hiding this comment

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

GG
Looks cleaner indeed!

Copy link
Collaborator

@erocheleau erocheleau left a comment

Choose a reason for hiding this comment

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

Some more suggestions, but we're getting really close!

packages/quantic/decisions/0001-testing-strategy.md Outdated Show resolved Hide resolved
packages/quantic/decisions/0001-testing-strategy.md Outdated Show resolved Hide resolved
packages/quantic/decisions/0001-testing-strategy.md Outdated Show resolved Hide resolved
packages/quantic/decisions/0001-testing-strategy.md Outdated Show resolved Hide resolved
packages/quantic/decisions/0001-testing-strategy.md Outdated Show resolved Hide resolved
packages/quantic/playwright/page-object/searchObject.ts Outdated Show resolved Hide resolved
Comment on lines +3 to +5
export const searchRequestRegex = /\/rest\/search\/v2\?organizationId=.*/;
export const insightSearchRequestRegex =
/\/rest\/organizations\/.*\/insight\/v1\/configs\/.*\/search$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either group these regex where you're using them, or put the isSearchRequest and isInsightSearchRequest here :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean by isSearchRequest and isInsightSearchRequest, are these functions I have somewhere? cause I don't think I have functions with this name

Copy link
Collaborator

@erocheleau erocheleau left a comment

Choose a reason for hiding this comment

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

Some suggestions still.

If possible, or at least try it to see if it doesn't explode the complexity of the files, but see if you can make the useCase an "option" that we could pass to the TestObjects and/or to the fixtures with the test.use instruction?

See if that way we can merge the files for Insight and Search useCases and instead of having two different fixtures with repeated code, we could have one fixtures that supports the two use-cases.

If you see the file becoming too complex, please say so, and we will keep those separate.

};

export const testSearch = quanticBase.extend<QuanticPagerE2ESearchFixtures>({
options: {},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these coming from the test.use ?

This is what I was thinking we could do to simplify the useCases too:

test.use({
    useCase: 'search|insight',
});

// And then in your fixture:
pager: async ({page, ..., useCase}, use) => {
    await use(new PagerObject(page, useCase))
}

// And then in your PagerObject, for example instead of having a param on 
async waitForPagerUaAnalytics() {
    if (this.useCase === 'insight') {...}
    if (this.useCase === 'search') {...}
}

Basically is there a way to not have two fixtures, testInsight & testSearch and simply have a testPager fixture that instead gets a useCase as an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the Pager Object is fully independent of the use case, it doesn't care at all about which use case you are is, which is my sense ideal. I'm not understanding why we would want to make it take a use case as a parameter?

Comment on lines +3 to +21
export class InsightSetupObject {
constructor(private page: Page) {
this.page = page;
}

get initializationInput(): Locator {
return this.page.locator(
'c-quantic-insight-interface input[type="hidden"]'
);
}

async waitForInsightInterfaceInitialization(): Promise<void> {
await this.initializationInput.waitFor({state: 'attached'});
await expect(this.initializationInput).toHaveAttribute(
'is-initialized',
'true'
);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible this could be in a single file with the searchObject that would receive a useCase as a param in the constructor?

Would you prefer this? This is just a suggestion trying to group the code that is different depending on the useCase in the same file, so we can re-use the exact same object in all tests
and the only thing that would change is the useCase we pass to the constructor?

Suggested change
export class InsightSetupObject {
constructor(private page: Page) {
this.page = page;
}
get initializationInput(): Locator {
return this.page.locator(
'c-quantic-insight-interface input[type="hidden"]'
);
}
async waitForInsightInterfaceInitialization(): Promise<void> {
await this.initializationInput.waitFor({state: 'attached'});
await expect(this.initializationInput).toHaveAttribute(
'is-initialized',
'true'
);
}
}
export class QuanticInterfaceObject {
private readonly initializationInput: Locator;
constructor(private page: Page, private useCase: string) {
this.page = page;
this.initializationInput = this.page.locator(
'c-quantic-insight-interface input[type="hidden"]'
);
}
async waitForInterfaceInitialization: Promise<void> {
if(this.useCase == 'insight') {
await this.waitForInsightInterfaceInitialization();
} else {
await this.waitForSearchInterfaceInitialization(); // add this
}
}
async waitForInsightInterfaceInitialization(): Promise<void> {
await this.initializationInput.waitFor({state: 'attached'});
await expect(this.initializationInput).toHaveAttribute(
'is-initialized',
'true'
);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal I'm aiming for in this setup is to separate the concerns, and have Classes that are responsible of one concise and clear objective, to keep simplicity and clarity.

That's why for example I have the class InsightSetupObject which takes care of a specific job of initializing an insight interface, this job is not needed for search so that's why I didn't mix it with search.

having QuanticInterfaceObject that groups handling the search and insight will motivate the presence of a code in a following shape:

   if(this.useCase == 'insight') {
      // do this
    } else {
      // do this
    }

and for me this does not separate the concerns in an ideal way, especially that we have other use cases like case assist and recommendations so following this logic we would also support them in QuanticInterfaceObject which I think will not scale really well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be part of the "ConfiguratorObject" since the Execute Search button is part of that component no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The executed search button is a standalone component that's passed to the configurator component as a slot,
but in term of functionalities, the ConfiguratorObject has one specific job which is to configure a given component with a given test environment, that's its responsibility it configures.

The Execute Search button does not configure, it triggers a real search query simulating clicking a search box button, to be used when we wanna test the behaviour of some components when a search request occurs, like for example what we are doing in the pager.

That's why for it doesn't belong to the ConfiguratorObject

packages/quantic/playwright.config.ts Outdated Show resolved Hide resolved
@mmitiche
Copy link
Contributor Author

Some suggestions still.

If possible, or at least try it to see if it doesn't explode the complexity of the files, but see if you can make the useCase an "option" that we could pass to the TestObjects and/or to the fixtures with the test.use instruction?

See if that way we can merge the files for Insight and Search useCases and instead of having two different fixtures with repeated code, we could have one fixtures that supports the two use-cases.

If you see the file becoming too complex, please say so, and we will keep those separate.

I don't see what advantage we have of merging fixtures or merging page objects by making them take a use case parameter.
do it that will increase the branching using if/else in our code increasing by this in my opinion the complexity and decreasing scalability as we will have other use cases to cover in our tests.

in the other hands I see pages objects that are independent of the use cases and and two fixtures, because each use case requires a specific different setup so it makes sense for me have a search fixture preparing the right setup for search and an insight fixture preparing the setup for insight in a clear manner that doesn't include a lot of branching.

Copy link
Collaborator

@erocheleau erocheleau left a comment

Choose a reason for hiding this comment

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

in the other hands I see pages objects that are independent of the use cases and and two fixtures, because each use case requires a specific different setup so it makes sense for me have a search fixture preparing the right setup for search and an insight fixture preparing the setup for insight in a clear manner that doesn't include a lot of branching.

That's true as well, I was thinking about the number of files it creates overall, but you're right it'll just make less files but more complex.

Let's do it!!

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.

5 participants