Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Follow-up to #1446366: [meta] Multiple web test classes mislabeled as unit tests. simpletest ([Web|Unit|DrupalUnit]TestBase)
InfoParserUnitTest web test classes mislabeled as unit tests
Proposed resolution
Remove "Unit" from the test name.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-20-22.txt | 1009 bytes | Ankit Agrawal |
#23 | infoparserunittest_web-2490388-22.patch | 11.62 KB | Ankit Agrawal |
#20 | infoparserunittest_web-2490388-20.patch | 11.63 KB | hussainweb |
#20 | interdiff-18-20.txt | 1009 bytes | hussainweb |
Comments
Comment #1
joelpittetHere's a rename.
Comment #2
dawehnerGiven the code we could also convert this test into a proper unit test using vfs, see
\Drupal\Tests\user\Unit\PermissionHandlerTest
as example.Comment #3
joelpittet@dawehner I added some non-unit test stuff to it I think. #2409515: Updater::findInfoFile() lacks test coverage
I made this issue so the rename didn't look messy. Maybe you have a better place for that though? I'm horrible at finding places to put tests.
Comment #4
lauriiiI've struggled with this too. I was debugging why I cant run this inside phpstorm and after all figured out that its not a actual unit test. The patch still applies. This is a little bit disruptive because patches changing that test needs reroll but I still think this is worth doing.
Comment #5
alexpottI agree with @dawehner let's convert this to a proper unit test.
Comment #6
joelpittetThanks for committing the other issue. That shouldn't be too trick(I hope) as long as things are mockable and self contained it should work. IIRC, may have had a
file_scan_directory()
call in there that may have made this tricky to do...I'll take this on in a while if nobody picks it up in the mean time.
Comment #7
hussainwebI know we are considering to put this back into an unit test, but I thought it would be good to have a reroll as a reference. Please set back to needs work once tests complete.
Comment #8
dawehnerWhat about converting it to a unit test instead? With vfs this should be totally be doable in a nice way!
Comment #9
hussainweb@dawehner: Yes, I am looking into that. I just wanted to get this patch rerolled to have as a reference.
Comment #10
hussainwebI have not used VFS here yet but this is a straight move to unit tests directory and changing it to use PHPUnit.
Comment #11
dawehnerIMHO we should move it to the core unit tests
We should use @expectedException ...
Comment #13
hussainwebJust moved the file again as per #11.1. I moved the second test in this class to a new file. I am trying out VFS with a small change first and then will go on to convert the whole test.
Comment #15
hussainwebOops. Wrong namespace.
Comment #17
dawehner@hussainweb Thank you for the effort!
Comment #18
hussainwebSo, it turns out that the test for Updater is not easy with PHPUnit. We need to mock a static method (
Updater::findInfoFile()
) to get it to work but that is not possible anymore. I moved it back to the regular test. Once this patch turns green, we could rename the file to clarify it's function and remove UnitTest from the name.The other test now uses vfs. The test method is now split into four different methods.
Comment #19
dawehnerOOH:
Yeah that does not belong into the unit test for this class. I'd suggest to maybe keep it in its current position.
Comment #20
hussainwebRenaming the file and removing an unused
use
.Comment #21
dawehnerOld doc ...
Comment #22
Ankit Agrawal CreditAttribution: Ankit Agrawal as a volunteer commentedPlease ignore this files.
Comment #23
Ankit Agrawal CreditAttribution: Ankit Agrawal as a volunteer commentedComment #25
dawehnerNice work!
Comment #26
alexpottNice work everyone. Committed c17a82f and pushed to 8.0.x. Thanks!