tests: improve playwright test readability #5149
Conversation
test/e2e/tools/step.js
Outdated
| * @param {string} text - The message to log, indicating the current step. | ||
| * @returns {void} Indicates that this function does not return a value. | ||
| */ | ||
| export function step(text) { |
There was a problem hiding this comment.
This somewhat mucks with the dot printing way of the node test runner to display its progress.
Since in the event of a failiour, the place where the failiour is is reported, I don't think this adds value.
=> please remove this part of the PR
If some of the comments around them can be improved via the work you put in, that is fair gain and what I would like to see instead.
Note though comments have to describe something non-trivial.
For example the following would not be a good comment
// Taking SQLite snapshot
await takeSqliteSnapshot(page);
test/e2e/specs/setup-process.once.js
Outdated
|
|
||
| test.describe("Uptime Kuma Setup", () => { | ||
|
|
||
| // Skip tests if the SQLite database already exists (only run once per session) |
There was a problem hiding this comment.
nit: does not add value imo
| // Skip tests if the SQLite database already exists (only run once per session) |
test/e2e/specs/monitor-form.spec.js
Outdated
| * @param {string} monitorType - The monitor type to select (default is "dns"). | ||
| * @returns {Promise<void>} - A promise that resolves when the form setup is complete. | ||
| */ | ||
| async function setupMonitorForm(page, testInfo, monitorType = "dns") { |
There was a problem hiding this comment.
About this function, please remove the parts which make this the testcases harder to read and duplicate that code instead.
DRY is not a goal for testcases, debuggability is.
Sorry if I was unclear about this in #5148 (comment)
For example the following would be much better for readability in a testcase
await page.goto("./add");
await login(page);
await screenshot(testInfo, page);
await selectMonitorType("dns")
test/e2e/specs/setup-process.once.js
Outdated
| /* | ||
| * Hook to take a screenshot after each test | ||
| */ |
There was a problem hiding this comment.
Think the code below is clear enough in this
| /* | |
| * Hook to take a screenshot after each test | |
| */ |
CommanderStorm
added
area:core
Thank you for the feedback! I'm learning and practicing as I go, and I’ve made the updates based on your suggestions. I hope everything looks good now, but feel free to let me know if anything else needs adjustment. Great working together on this :) |
CommanderStorm
changed the title
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma
Tick the checkbox if you understand [x]:
Description
This pull request improves the test suites by adding step() markers across tests for better debugging in the future. I also refactored the code to follow the DRY principle, reducing redundancy and improving maintainability.
Type of change
Checklist
Screenshots (if any)
Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.