wp-media / wp-rocket-e2e

Playwright E2E testing repo
GNU General Public License v3.0
0 stars 0 forks source link

Switch LCP tests from API to test file #71

Closed Miraeld closed 4 months ago

Miraeld commented 4 months ago

Is your feature request related to a problem? Please describe.

The PSI API we use in the LCP tests isn't something we should do. In fact an API doesn't guarantee to be available at any moment + it sometimes doesn't respond the same answer at each try.

Describe the solution you'd like

We could generate a file which will take for granted as expected output for LCP + ATF. It could be generated at first from the PSI API, and later on reviewed, updated by the product/QA/Dev team

Describe alternatives you've considered

Additional context

We already expect some templates on Rocket-E2E for LCP/ATF to fail against the PSI API. They have been manually investigated in the Slack channel #wpr-316-template-investigations. So the manual modifications to apply to the file of expected results can be guided based on those observations for each failing template. We already have a file of expected results for the ATF. So we could reuse the same mechanism.

Miraeld commented 4 months ago

Scope a solution

To implement this feature, we must generate results files. I link two files which are respectively the expected results for desktop and mobile. ⚠ ‼ ⚠ Files have been generated by using the PSI API for LCP results. And they haven't been corrected when these ones are wrong. Please make sure to updated the results according to what we are expecting per template @wp-media/productrocket @wp-media/qa-team

expectedResultsDesktop.json expectedResultsMobile.json

We will have to load files path:

const lcpResultsDesktop = './support/results/expectedResultsDesktop.json';
const lcpResultsMobile = './support/results/expectedResultsMobile.json';

and use them with (to be adapter in our case for both file):

    const data = await fs.readFile(lcpResultsDesktop, 'utf8');
    const jsonData = JSON.parse(data);

Finally, we will need to adapt this part https://github.com/wp-media/wp-rocket-e2e/blob/64648aaf5ab8a599febff3792e849ab40983d51d/src/support/steps/lcp-beacon-script.ts#L87-L113

to use our file instead of the API.

Also, something important, as our JSON files are containing the list of templates to test, we don't need to keep a datatable within the I visit the following urls in {string} step (https://github.com/wp-media/wp-rocket-e2e/blob/trunk/src/features/lcp-beacon-script.feature#L13-L39)

Development steps:

Effort estimation: S/M

Can be peer-coded:No

Is a refactor needed in that part of the codebase?

No

jeawhanlee commented 4 months ago

I see we already have viewport expectations in the JSON file, we need to remove the step that adds them in the datatable here and here and by this we will need to update the steps.

We don't need the fs module to use the JSON file, we already support JSON resolving here or the path could just be directly called like we do here

After updating the grooming, LGTM

Miraeld commented 4 months ago

@jeawhanlee , I'm not sure we should delete the viewport expectation from the steps. In a way, removing it means in the code we will have to process both Desktop & Mobile together, while keeping it allows you to start either one of them. Which could be handy in the future if we need to run only desktop for example, for whatever other test we have to do.

Don't you think ?

Otherwise, I've updated the grooming according your feedback

jeawhanlee commented 4 months ago

Not to delete the step but since we have a JSON file now, we should port all the viewport expectations there and updated the step to receive no datatable argument.

Miraeld commented 4 months ago

I've edited the grooming to add a note about this.

Miraeld commented 4 months ago

@piotrbak or @Mai-Saad / @hanna-meda Could it be possible for you to have a look to the two JSON Files linked to the grooming and correct what's was badly detected by Google PSI API. While waiting for this, I put this issue as Blocked as we can't do anything without a corrected version. (I've corrected some, we just want a second look and confirmation this is ok or not).

Mai-Saad commented 4 months ago

@Miraeld Thanks for the ticket. Please find updated files. Thanks @hanna-meda for updating the files expectedResultsMobile_revised_updated.json expectedResultsDesktop_revised_updated.json

Miraeld commented 4 months ago

@Mai-Saad thanks for the revisited version, I'll be able to finish the PR :)