ci(e2e): added config to handle carousel

[inc-man]

Motivation

Since the implementation of the Dapps Carousel, the e2e tests were inconsistent. Due to the carousel automatically switching slides there was a possibility for screenshot 1 to be on slide x and screenshot 2 to be on slide y. For a consistent and useful test this needs to be changed.

Changes

• added config that whenever the test for the homepage is run, it will check the carousel slides and always take the screenshot on the last slide

Tests

tested multiple times on local instance.

[inc-man]

@AntonioVentilii @DenysKarmazynDFINITY

Please have a look at what I did. Maybe I overdid some things or forgot others, but like this I get a consistent result on my local oisy instance.

[AntonioVentilii]

A few comments, but, in general, I thought we were going to "deactivate" the carousel for all E2E tests, and test the carousel in another separate test, "reactivating" it only once for that specific test.

I think it would even make sense performance-wise, no? Did I misunderstand? WDYT?

[AntonioVentilii]

ideally, this i would put inside the .waitForReady, for the simple fact that it will be useful for other screenshots too (for example explorer and activity page)

however, make it a specific function to be called, so that we can create a test to test the carousel separately

for example (random naming):

async testCarousel = () => {
  const numberOfSlides = await homepageLoggedIn.getNumberOfSlides();
  for (let i = 1; i <= numberOfSlides; i++) {
    await homepageLoggedIn.navigateToSlide(i);
  }
}

async waitForReady {
  ... // existing implementation
  await homepageLoggedIn.navigateToSlide(0); // only one slide
}

[inc-man]

I did not put it somewhere else because the carousel is only showing on the homepage. I mean yes it does show when it tests for specific modals but activity page for example does not show the carousel.

I don't yet see why we would want to test the carousel seperately.. Since it is part of the "homepage" imo its totally fine to test it there.

[AntonioVentilii]

well, we have quite a lot of other tests that inherits the homepage (receive, settings, activity, explorer, for example). All of those need a stable carousel too, and the only thing that they have in common at start is waitForReady (that makes sense)

[AntonioVentilii]

in fact, if you are going to add this in this test, you should add for all of them, now that I think about it...

[AntonioVentilii]

let's put it in a folder utils/components

[inc-man]

that I will do, probably gonna need more components from time to time

[AntonioVentilii]

shall we keep the same convention, e.g. #page: Page, #container: Locator, etc

plus, i think you can remove the carousel prefix, since they are part of the class already

[inc-man]

so we would do

#page
#container
#slideNavigation

or something else?

[AntonioVentilii]

i think that is ok, no?

#page
#container
#slideNavigation

[AntonioVentilii]

just #carousel ?

[inc-man]

I mean what if we add a new carousel at some point? better have a consistent naming now instead of renaming all the carousels after no?

[AntonioVentilii]

ah good point! I didn't know we had that in our plans eheh!

[AntonioVentilii]

these two function could go theoretically directly inside the class of the carousel, so that you call them directly from the object

[AntonioVentilii]

you don't need to re-initiate it here right? it is done directly in super({ page, viewportSize });

[inc-man]

I think I did it the way I found it. iiPage is re-initiated aswell right? Or am I missing something? But in general you are correct, it could be just put into super. Then again we only need the carousel in homepageloggedin, since it wont display in loggedout

[AntonioVentilii]

no, what i meant is that it is already initiated in super, you are re-initiating it, no? isn't it a duplicated code?

[AntonioVentilii]

do we want to initiate it here? shouldn't it be inside some sort of awaiting function? so that we can avoid fails on initiation, but we check for the existence of the carousel too

[DenysKarmazynDFINITY]

I may miss some context, but why do we need to navigate to every slide via a loop when we are probably interested in just having the last one displayed? Would a simple await homepageLoggedIn.navigateToSlide(numberOfSlides); not work in this case?

[inc-man]

I thought this would be a better solution. Because basically we not only test if the carousel exists but also if the navigation works. test improvement imo

[DenysKarmazynDFINITY]

I see your point. It would be nice to create separate tests for the carousel to make sure that other parts are functional too (navigation via errors, last-to-first and first-to-last slides switches, autoplay, etc.) and keep this particular test suite only about logged-in page screenshot. For now, I'm fine keeping it as it is (1 carouse test is better than 0 carousel tests 🙂 ), but please add a todo comment to move the loop out + add additional coverage for the component.