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

Enhancement/11 tests to cover number of ajax calls #17

Merged

Conversation

MathieuLamiot
Copy link
Contributor

Description

Follow-up of #11 and #13 by adding tests now that the CI works. The tests ensure we don't do useless AJAX calls.

Documentation

User documentation

No user impact.

Technical documentation

Added the following tests on BeaconManager:

  • should not send AJAX calls if invalid preconditions
  • should not send AJAX calls if not cached page
  • should not send AJAX save data if data already generated
  • should not send AJAX save data if no features ran

Type of change

Delete options that are not relevant.

  • New feature (non-breaking change which adds functionality).
  • Bug fix (non-breaking change which fixes an issue).
  • Enhancement (non-breaking change which improves an existing functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as before).

New dependencies

NA

Risks

NA

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.

@MathieuLamiot
Copy link
Contributor Author

@Mai-Saad The tests in this PR don't check exactly the same thing as in the test case you shared, as it is not an E2E test but an integration test. I think we can adapt the test case on Test rail to reflect the automated test, WDYT?

Here is what the code checks:

  • With invalid Preconditions, there are no AJAX calls.
  • With valid preconditions and page not cached, the beacon does the check_data AJAX call once.
  • With valid preconditions and data already generated (based on 1st AJAX call), there are no further calls.
  • With valid preconditions and no optimization running (because disabled by the user and/or data already tehre), there are no further calls.

valid preconditions means screensize is acceptable. But this was not tested properly so I pushed commits with tests on the screensize too. So that we can consider the "valid preconditions" assertion correct.
Let me know what you think.

@Mai-Saad
Copy link

@MathieuLamiot Thanks for the feedback. Updated related old tests to check ajax not sent while script injection is not done with invalid screen sizes, data already generated.. for no optimization running, it will be considered in related test steps when disable all performance hints features.

Shouldn't we depend on integration tests once PR is merged and not manually checking that?

@MathieuLamiot
Copy link
Contributor Author

👍

Shouldn't we depend on integration tests once PR is merged and not manually checking that?

This would be manually checked once as part of Acceptance Criteria, and then I believe we can rely on integration tests only

@MathieuLamiot MathieuLamiot merged commit d0cf229 into develop Aug 16, 2024
2 checks passed
@wordpressfan wordpressfan deleted the enhancement/11-tests-to-cover-number-of-AJAX-calls branch August 19, 2024 07:07
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.

3 participants