ladybird icon indicating copy to clipboard operation
ladybird copied to clipboard

LibWeb/Tests: Add Ref, Layout, and Screenshot test support to `add_libweb_test.py`

Open noahmbright opened this issue 1 year ago • 6 comments

The boilerplate could probably be more helpful for the newer test types. I can fix that up if given recommendations/requests. I found the existing tests a bit irregular 😅

Also updated the docs to reflect the changes. While it's topical, I can also write some stuff up addressing #398. I already have some stubbed in sections for the new test types added in the doc ("the different kinds of tests" to quote the issue), and I myself would like to know more about "what makes a test good". I'll summarize whatever else if given something to work from

noahmbright avatar Oct 09 '24 21:10 noahmbright

Another thing I noticed - currently Ref and Screenshot tests can't be in subdirectories. It might be nice if this script told you so, instead of creating tests that don't get run.

I'll fix this but I'm not understanding the mis-use case yet. What could you do incorrectly that would make a test that doesn't get run?

noahmbright avatar Oct 16 '24 14:10 noahmbright

I'll fix this but I'm not understanding the mis-use case yet. What could you do incorrectly that would make a test that doesn't get run?

Tests/LibWeb/add_libweb_test.py some/subdir/test.html ref creates Tests/LibWeb/Ref/some/subdir/test.html, but only test files directly in Tests/LibWeb/Ref are found by the test runner, so this new test is invisible to it. Same for screenshot tests. It's a quirk of them not having an input/ directory. Maybe it makes most sense to give them an input/ directory instead, but that's out of scope for this PR.

Feel free to just ignore the subdirectory thing!

AtkinsSJ avatar Oct 17 '24 08:10 AtkinsSJ

Tests/LibWeb/add_libweb_test.py some/subdir/test.html ref creates Tests/LibWeb/Ref/some/subdir/test.html

Okay I see, I only considered someone putting in test-name as opposed to path/to/test_name. We could add --subdir as an extra flag and check for valid combinations. Not sure if you'd prefer to freely make new directories or if you want the fat finger insurance. Could also defer that to another commit.

noahmbright avatar Oct 17 '24 10:10 noahmbright

This has a conflict - the documentation changes now need to go in Documentation/Testing.md.

Worrying about subdirectories can be deferred for now. I think making those test types allow subdirectories would be the better option, which is out of scope for your PR.

AtkinsSJ avatar Nov 01 '24 10:11 AtkinsSJ

In #2175 I resolved the subdirectory issue by making Ref/Screenshot tests use the same directory structure as the others (allowing tests in subdirectories).

However, the tests now go in Ref/input and the expectations in Ref/expected instead of Ref/reference, (and same for Screenshot tests) so those paths need updating here.

AtkinsSJ avatar Nov 05 '24 14:11 AtkinsSJ

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

github-actions[bot] avatar Dec 07 '24 21:12 github-actions[bot]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

github-actions[bot] avatar Dec 16 '24 02:12 github-actions[bot]