test(SD-960): add automated SD export + reimport visual tests #1293
Conversation
|
@harbournick / @chittolinag Can you please review this? Please bear with me as this is the first time we are involved in complex tests and might need more guidance if anything is missing in here |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| import path from 'path'; | ||
| import { PNG } from 'pngjs'; | ||
| import pixelmatch from 'pixelmatch'; | ||
| import config from '../../test-config'; |
There was a problem hiding this comment.
The new visual spec imports ../../test-config, but there is no test-config module anywhere under e2e-tests (verified with find e2e-tests -maxdepth 3 -type f -name 'test-config.*'), so loading the spec will throw Cannot find module '../../test-config' before any tests execute. The suite cannot run until the config file is added or the import path corrected.
Useful? React with 👍 / 👎.
| import { test, expect } from '@playwright/test'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { PNG } from 'pngjs'; | ||
| import pixelmatch from 'pixelmatch'; |
There was a problem hiding this comment.
The spec imports @playwright/test, pngjs, and pixelmatch, but none of these packages are declared in the repository manifests (searched the root/workspace package.json and package-lock for those names), so any attempt to run this file will fail module resolution before the tests start, even if the missing config file is added. Please declare the required dependencies or place the test under a package that already provides them.
Useful? React with 👍 / 👎.
chittolinag
left a comment
chittolinag
left a comment
There was a problem hiding this comment.
Hey @priya-harbour ! Thanks for helping here. I think there's a way to simplify this logic. Instead of comparing the screenshots manually, we can leverage the toHaveScreenshot function that Playwright provides. We would very likely be able to remove completely the compareScreenshotWithSnapshot function, which would make it easier to maintain down the road. Also, as a side note, the pipeline is failing (see execution failure here: https://github.com/Harbour-Enterprises/SuperDoc/actions/runs/19550483233/job/55980395348?pr=1293
Let's talk about this one next week and I can help you here. Thanks!
…trip tests - Added newly generated visual snapshots for all basic-documents - Synced round-trip export → reimport flow to current SuperDoc behavior - Added tiny-spacing to IGNORED_DOCUMENTS (exportDocx returns undefined; tracked via FIXME)
There was a problem hiding this comment.
is this one correct @priya-harbour ? it seems like it's empty, just to confirm we're not missing anything here
There was a problem hiding this comment.
Hey @chittolinag , No, I am updating the screenshots, so I will ping you once everything is ready
|
@priya-harbour the PR looks good to me! I just left one comment on a file, please take a look when you have some time. also, let's hold merging this one because Nick is working on making visual diffing + layout engine to work together, and hence we can come back to this PR and merge it. |
Hey @chittolinag , thank you. Yes, I am tweaking a little bit with new screenshots, etc., so I will wait until Nick's fixes are also in and will let you know after everything is ready |
SD-960: Automated Visual Testing for SD Export → Reimport
This PR introduces a new Playwright-based visual regression suite
to detect formatting regressions when a document is exported from SD
and re-imported back into SD.
What This Test Does
basic-documentsWhy This Is Needed
We recently had a regression where documents with no strikethrough
were exported and re-imported with unexpected strikethrough formatting.
This test ensures such regressions will be caught automatically.
Included
sd-export-reimport.spec.js*.png)