Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Setup some simpletest for the tracker module and noticed dmitrig01 is doing some work on it as well. Maybe the tests that I created can help dmitrig01 out.
Comment | File | Size | Author |
---|---|---|---|
#12 | tracker.test.patch | 4.8 KB | boombatower |
#11 | tracker.test.patch | 4.75 KB | floretan |
#9 | tracker.test.patch | 4.13 KB | floretan |
#5 | tracker_tests.patch | 10.04 KB | dmitrig01 |
#4 | tracker_tests.patch | 1.02 KB | dmitrig01 |
Comments
Comment #1
skywalker2208 CreditAttribution: skywalker2208 commentedForgot to add the test file.
Comment #2
webchickMarking needs review. No time to do it myself atm.
Comment #3
boombatower CreditAttribution: boombatower commentedI was hoping dmitrig01 would post his and we could compare/merge, but that doesn't seem to be happening so I'll review yours tomorrow.
Comment #4
dmitrig01 CreditAttribution: dmitrig01 commentedHere they are, my very special tracker tests.
Comment #5
dmitrig01 CreditAttribution: dmitrig01 commentedWith the tests this time!
Comment #6
boombatower CreditAttribution: boombatower commented@dmitrig01: I noticed a few things that I would appreciate if they were cleaned up.
drupalCreateNodes
method is necessary, and if it is I would like to have others approve it. The comment has a bit of random spacing and is missing periods.t()
.tearDown
just to call parent.setUp
.You can look at the tests that I have reviewed to see an example of what we are looking for.
The changes are minor, but the test as a whole looks nice.
Thanks.
Comment #7
catchhttp://drupal.org/node/253500 was duplicate.
Comment #8
Dries CreditAttribution: Dries commentedI think boombatower's review still stands.
Comment #9
floretan CreditAttribution: floretan commentedI agree with boombatower's comments. The patch from #5 is also not structured like any existing core texts:
* Its use of xpath is very powerful, but also very obscure.
* Direct calls to curlExec() are a big no-no, they belong in the testing framework, not in the tests themselves.
* Why do we need to test everything on multiple nodes at once? Unless we're looking for a specific behavior when dealing with multiple nodes, we shouldn't test on multiple nodes. There's no need to add unrelated parameters to a test.
Here's a simpler test that covers the same functionality.
Comment #10
floretan CreditAttribution: floretan commentedAlthough the patch from #1 needs some clean-up, it covers some code that is not covered by dmitrig01's patch or mine. I'll work on merging these tests.
Comment #11
floretan CreditAttribution: floretan commentedAfter a closer look, the only case checked by the patch from #1 that I was missing was that unpublished nodes should not show up in the tracker listing.
Comment #12
boombatower CreditAttribution: boombatower commentedThis patch is much cleaner than the original. It passes on my dev box and seems to be much more consistent with other tests.
I removed several page content outputs that seem to be left over from development and cleaned up some white-spacing.
Good work flobruit.
Comment #13
boombatower CreditAttribution: boombatower commentedChange component is relation to http://drupal.org/node/253744.
Comment #14
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks all.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.