Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Folowup from #1782838: WYSIWYG in core: round one — filter types
Comment | File | Size | Author |
---|---|---|---|
#5 | 1816162-5.patch | 2.63 KB | Wim Leers |
Comments
Comment #1
Wim LeersSpecifically, from #1782838-49: WYSIWYG in core: round one — filter types.
Comment #2
Crell CreditAttribution: Crell commentedMy comment from the previous thread:
This should be two separate tests. Merging two tests into a single test method is a moderately large testing faux pas.
Comment #3
Crell CreditAttribution: Crell commentedTo clarify per Wim's request, this method is testing both $expected_filtered_text and the "without html generators" situations. That's wrong. There should be one test that sets up and runs the first assertIdentical(), and a second test that sets up and runs the second assertIdentical(). Yes, there will be some overlap between those methods. That's OK. In unit tests, repeated code between methods is OK (or can be factored to a utility method); the focus is on making each test method as focused and single-purpose as possible, so that if something fails you get good fine-grained results.
And of course these should all be unit tests, not web tests, but that may not be possible until the code is refactored to be OO post-December.
Comment #4
Wim LeersGotcha. I've always scoped unit tests differently, the same way as Qt's tests do it: one test method per method/function that you want to test, and that test method will then cover all possible cases (e.g. if the method accepts an int, then it tries -1, 0, 1, 1000, and MAX_INT-1 or so). Either way works for me.
IIRC I tried to make it a UnitTestCase subclass, but that failed, IIRC because check_markup() is not available for unit tests.
Comment #5
Wim Leers1) Making this a unit test is impossible AFAICT. Unit tests imply no DB access;
check_markup()
relies onfilter_formats()
which access the DB. It is possible to work around that (e.g. by filling its static cache), but I doubt that's acceptable in core.2) Implemented what you asked for in #3. Also fixed a typo.
3) Could you please point me to docs that state what you said in #3 is indeed required for Drupal core tests? If not, we should create that documentation, or otherwise others might do it the same way I did it.