Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

🎨 WP Directives plugin - Stress testing in real sites #91

Closed

Conversation

michalczaplinski
Copy link
Collaborator

@michalczaplinski michalczaplinski commented Oct 24, 2022

Note: If someone wants to help with the tests, please send me a DM on Slack (wordpress.slack.com) and I'll send you a link with the csv files that contain the list of sites.


This PR adds a script that runs some stress tests against multiple live WordPress pages and uses the MutationObserver to check for DOM modifications.

2022-10-27_13-10-17.mp4

@michalczaplinski michalczaplinski changed the title WIP: Add performance and stress testing Add MutationObserver tests Oct 27, 2022
@michalczaplinski michalczaplinski marked this pull request as ready for review October 27, 2022 02:27
@michalczaplinski
Copy link
Collaborator Author

michalczaplinski commented Oct 27, 2022

The latest results are in the results_first_100.jsonfile.

I'm currently focused on this:

  • Parallelize the tests so that we can test more sites quickly (currently they take a few mins for just the first 100 sites on my M1 Macbook.
  • Save the data into an sqlite database instead of into a JSON file. Looking at JSON files gets unwieldy at scale. This way we can:
    • Run the tests for the first 100 pages, save them to database and push to github. Then, we can resume testing next day without re-running the tests for the same first 100 sites.
    • We can use a tool like https://datasette.io/ to quickly look at the results.
    • We can keep track of which sites were tested and which ones resulted in an error. Not impossible with JSON but a bit harder to keep track of.

@SantosGuillamot
Copy link
Contributor

This looks great Michal! I made some Pull Requests with potential small improvements. Maybe some of them don't make much sense, just let me know 🙂

@michalczaplinski
Copy link
Collaborator Author

I've opened a PR to bundle the testing script with webpack instead of esbuild:

@cbravobernal
Copy link
Collaborator

I did the script to get the posts with 1.csv and I'm now running 2.csv

…age-step

Script to get a post using Rest API to test it with the vDOM script.
@SantosGuillamot
Copy link
Contributor

I started retesting 4000 sites, and I encountered the same hydrationError in 8 of them. It seems that the script fails if there is a ref attribute in the HTML. I’ve opened an issue to discuss it with more information - #116.

@cbravobernal
Copy link
Collaborator

Running get posts on 3.csv

@SantosGuillamot
Copy link
Contributor

SantosGuillamot commented Jan 10, 2023

We have tested all the sites and solved all the issues we encountered. Here's a summary and a recap of the process. Looking at the results, I believe we should feel confident enough for this initial phase.

Results

Result Count % Info
Success 145720 83.79 Everything went fine
Cloudflare 21165 12.17 Skipped to avoid Cloudflare banning on our IPs
Timeout error 3080 1.77 Skipped because the request timed out
Error 3025 1.75 Skipped because the request didn’t return a valid response
403 error 929 0.53 Skipped because the request returned a 403 (forbidden)
Hydration error 0 0 The hydration algorithm failed
Mutations 9 0 The hydration algorithm produced DOM mutations
Total 173919 100 Total sites tested

Main points

This is a list of the main issues we faced and the solutions proposed. We also iterated our testing script many times, but I’m not sharing anything related to that here.

  • Remove comments: Preact doesn’t support them during the hydration so we created this Pull Request to remove them. We may need to add them back after the hydration if sites rely on them.
  • Nigloland.fr site issue: This site is using a script that detects the removal of the comments with DOMSubtreeModified and makes extra mutations that are detected by our MutationObserver. We shouldn’t worry about it, but ideally we should preserve the comments.
  • The less we rely on native methods, the better: 24life.ro modifies String.prototype.startsWith, which caused some mutations. As Luis suggests in the comment, the less we rely on native methods, the better. This was solved in this pull request.
  • CDATA nodes: We hadn’t taken into account the CDATA nodes ( nodeType 4), so they were returning some errors and registering mutations. As David explains here, they are not allowed in HTML documents, but they are used inside SVGs. To solve it, we are converting them into Text nodes, implemented in this pull request.
  • xpacket nodes: Similar to the previous point, we hadn’t yet considered the ProcessingInstruction nodes (nodeType 7), and it was returning a hydration error. Again, it is not HTML, but it can be found in XML elements like SVGs. We are treating them like comments, implemented in this pull request.
  • ref attributes: We also encountered that many sites were returning an error when there was a ref attribute in the content. At the end, we decided to ignore them while generating virtual nodes.

@SantosGuillamot
Copy link
Contributor

As explained in the issue, I believe we can consider this initial testing phase completed and successful, so I'm closing the PR as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants