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.

Sign up for GitHub

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

Jump to bottom

Closes #71: Use results file instead of API for lcps #77

Merged
merged 16 commits into from
May 22, 2024
Merged

Closes #71: Use results file instead of API for lcps #77

merged 16 commits into from
May 22, 2024

Conversation

Miraeld
Copy link
Contributor

@Miraeld Miraeld commented May 10, 2024

Description

Fixes #71

Documentation

User documentation

Technical documentation

This PR changes the way we are testing the result from the beacon script. In fact, before we were using Google PSI API, while now we are using a file to compare our results to the expected one.
This allow us to faster, more reliable.

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

New dependencies

Risks

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Documentation

  • I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
  • The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
  • I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitely.
  • I protected entry points against unexpected inputs.
  • I did not introduce unecessary complexity.
  • I listed the introduced external dependencies explicitely on the PR.
  • I validated the repo-specific guidelines from CONTRIBUTING.md.

Observability

  • I handled errors when needed.
  •  I wrote user-facing messages that are understandable and provide actionable feedbacks.
  • I prepared ways to observe the implemented system (logs, data, etc.).

Risks

  •  I explicitely mentioned performance risks in the PR.
  • I explicitely mentioned security risks in the PR.

@Miraeld Miraeld added the enhancement New feature or request label May 10, 2024
@Miraeld Miraeld requested a review from jeawhanlee May 10, 2024 04:49
@Miraeld Miraeld self-assigned this May 10, 2024
jeawhanlee
jeawhanlee requested changes May 10, 2024
View reviewed changes
src/support/steps/lcp-beacon-script.ts Show resolved Hide resolved
src/support/steps/lcp-beacon-script.ts Outdated Show resolved Hide resolved
src/support/steps/lcp-beacon-script.ts Show resolved Hide resolved
src/support/steps/lcp-beacon-script.ts Outdated Show resolved Hide resolved
src/support/steps/lcp-beacon-script.ts Outdated Show resolved Hide resolved
src/support/steps/lcp-beacon-script.ts Show resolved Hide resolved
@Miraeld Miraeld requested a review from jeawhanlee May 10, 2024 21:06
@Miraeld Miraeld force-pushed the enhancement/71-use-result-file-for-lcp-tests branch from 841cb73 to 8ac81cf Compare May 10, 2024 21:15
@Miraeld
Copy link
Contributor Author

Miraeld commented May 13, 2024

Thia last commit corrects the expected results for Desktop.
Everything pass for the desktop.

jeawhanlee
jeawhanlee requested changes May 13, 2024
View reviewed changes
src/support/steps/lcp-beacon-script.ts Outdated Show resolved Hide resolved
src/support/steps/lcp-beacon-script.ts Outdated Show resolved Hide resolved
src/support/steps/lcp-beacon-script.ts Outdated Show resolved Hide resolved
Miraeld added 3 commits May 14, 2024 08:45
@Miraeld
Updates expected results
1cc1966 
Add is_mobile is sql query
Accept array for lcps
@Miraeld
Comply linter
5b62efe
@Miraeld
Comply feedback
bfd38d0
@Miraeld Miraeld force-pushed the enhancement/71-use-result-file-for-lcp-tests branch from 16c17d0 to bfd38d0 Compare May 14, 2024 06:53
@Miraeld
Copy link
Contributor Author

Miraeld commented May 14, 2024

Okay so,
@jeawhanlee I've updated with the last few commits the following:

  • Updated the JSON files based on QA feedbacks.
  • Introduced a enabled attribute for each entry of the JSON (as we can't comment out in a JSON). This attribute will allow us to enable or disable a URL in the test.
  • I've modified the LCP structure to be able to get an array as sometime we get several lcps for an img set.

Everything is working and passing, all green on e2e. :D

@Miraeld Miraeld requested a review from jeawhanlee May 14, 2024 06:54
@Miraeld Miraeld changed the title DRAFT: Closes #71: Use results file instead of API for lcps Closes #71: Use results file instead of API for lcps May 14, 2024
jeawhanlee
jeawhanlee previously approved these changes May 14, 2024
View reviewed changes
Mai-Saad
Mai-Saad reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

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

"lcp_single_double": has no value in json. @Miraeld can you please check

src/support/results/expectedResultsMobile.json Show resolved Hide resolved
src/support/results/expectedResultsDesktop.json Show resolved Hide resolved
@Mai-Saad
Copy link
Contributor

Mai-Saad commented May 15, 2024 •
edited
Loading

@jeawhanlee thanks for the update. 2 tests failed and many has no data in database => can you please check? (I think we need to see if it's the same before the PR and while using API)
Screenshot from 2024-05-15 17-28-51
Screenshot from 2024-05-15 17-31-11

@jeawhanlee
Copy link
Contributor

@Mai-Saad I think I found a reason for the no result in db, since you are on e2e env, your WP_SSH_ROOT_DIR should end with /htdocs

jeawhanlee and others added 3 commits May 16, 2024 09:17
@jeawhanlee
Updated hardcoded atf urls to relative (#81)
e91b6d2
@Khadreal
Closes #70 Add scenario for pages without LCP/ATF (#76)
18509cf 
* Add scenario for no lcp/atf, add plugin deactivation function #70 :closes:

* fixed lint

* minor fix

* Add step to activate and deactivate mobile plugin

* update gitignore file

* PR modifications

* Add new command function, pr modifications --#70

* minor modifications

* Add beacon check for lcp pages

* deactivate plugin, modify json

* Add missing step

* change steps
@jeawhanlee
Closes #82: LCP test not failing with no result in db (#84)
f618f94 
* Make tests fail when no result in db

* Fail tests early

* Make log red
@jeawhanlee
Copy link
Contributor

@Mai-Saad I think we can merge this PR since the failing mobile scenario is to be fixed here

@jeawhanlee jeawhanlee mentioned this pull request May 21, 2024
Merged
7 tasks
@Miraeld
Copy link
Contributor Author

Miraeld commented May 22, 2024

@jeawhanlee Should I merge it ?

@jeawhanlee jeawhanlee merged commit fb0c0e6 into trunk May 22, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeawhanlee jeawhanlee jeawhanlee approved these changes

@Mai-Saad Mai-Saad Awaiting requested review from Mai-Saad

Assignees

@Miraeld Miraeld

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Switch LCP tests from API to test file
4 participants
@Miraeld @Mai-Saad @jeawhanlee @Khadreal