This information is from the code coverage report (see http://coverage.cwgordon.com/coverage).
We need to test:
1) Viewing aggregator/categories/* pages. Addresses by post 52
2) Appropriate aggregator feeds periodically refreshing. Currently working in D8 module (AggregatorCronTest.php)
3) The aggregator module's provided blocks. Currently working in D8 module (AggregatorRenderingTest.php)
4) aggregator_element_data().
5) aggregator_refresh() with specified etag and modified.
6) Update with no new syndicated content.
7) Update to a moved feed.
8) Make sure lines 607-609 use theme('image') instead of other stupid stuff.
9) Update to unparsable feeds.
10) aggregator_parse_w3cdtf()
11) Trying to use aggregator_parse_feed() to parse invalid xml.
12) Update a feed with more than one new item.
13) Attempt to update to a feed with a duplicate item.
14) Update to a feed in a category.
15) View feed items in a category.
Comment | File | Size | Author |
---|---|---|---|
#56 | 276493-56.patch | 4.25 KB | lokapujya |
#53 | aggregator-test-category-pages-276493-52.patch | 2.2 KB | Pat Redmond |
#45 | 276493-29_aggregator_tests.patch | 23.29 KB | Berdir |
#37 | 276493-28_aggregator_tests.patch | 23.42 KB | Berdir |
#27 | 276493-27_aggregator_tests.patch | 19.41 KB | grndlvl |
Comments
Comment #1
BerdirOk, attached is a first version of updated tests.
Note: Currently, the categorize tests *will* fail, this is because of : #423690: menu_link_maintain(), $op update critically broken.
Contained tests:
- Feed and Category blocks (checks existence, activating, title and feed items)
- Category page, including title and feed items check
- Update a category (this will "fail")
- Delete a category
Untouched functions :
- aggregator_cron (I have to figure out what that actually does)
- aggregator_sanitize_configuration: Will do that later
- aggregator_block_configure/aggregator_block_save- Need to test the block settings
I cheated (many stuff of the file has been moved to other files), but aggregator.module is now at 85% code coverage, which is a start :)
modules/aggregator/aggregator.module 387 65 85.62%
This not finished, but I'm setting it to review so that the tests can run.
Comment #3
BerdirSome more tests.
Note: I've found another bug that needs to be fixed to get these tests to pass. This one is in aggregator.module, but I created a new issue so that it can easily be fixed while the tests can be improved. See #423886: aggregator_block_save fails with "Field 'description' doesn't have a default value". This patch will have multiple fails because of that bug.
With the latest patch, code coverage is now at 95% and aggregator.test has 805 test assertions.
I still need to add some apidocs and probably inline docs to some stuff.
Comment #5
webchick#423886: aggregator_block_save fails with "Field 'description' doesn't have a default value" is in now. Let's try this again.
Comment #6
alex_b CreditAttribution: alex_b commented#293318: Convert Aggregator feeds into entities contains a test module and tests for fetcher, parser and processor plugins.
I see that you're testing another aspect of the pluggable architecture though: the sanitize() function. So, right now, there is no overlap.
If ever such tests should be part of this patch though, it would be great if you could copy them from there - this would make my job of keeping 293318 up to date muuuch easier.
Need to run now, will review patch later in more detail.
Comment #7
Dries CreditAttribution: Dries commentedI'm delegating this patch to alex_b because his work on aggregator module. I will commit this patch (after a quick review) when alex_b thinks this patch is ready. Thanks!
Comment #8
Berdir@alex_b: I tried to focus on testing functions inside the file aggregator.module. That mainly includes hooks (especially block), api functions and also the sanitize function.
Comment #9
Berdiraggregator.module has changed a lot since the initial list was made, but of that list, the following things are tested in this issue:
Also tested:
- API functions, for saving, updating and deleting categories
- sanitize function that checks if the configured "handlers" for fetching, parsing etc. are available and if not, resets to default
Comment #10
BerdirAdded some comments and remove self::$prefix, as we test in a separate db now, there is no reason to use some sort of prefix imho.
Comment #11
brianV CreditAttribution: brianV commentedI wrote an aggregator.module patch that needs a test written in #404356: Aggregator should strip tags before using beginning of description tag as title. Unfortunately, I haven't completely figured the test interface out - I would really appreciate it if someone wouldn't mind writing a test for the patch.
Comment #12
alex_b CreditAttribution: alex_b commented@Berdir, thanks for the updated list.
The tests you've added are looking very good, nice work. Here is an item by item break down that I did following your list and looking at the patch:
1) *DONE* Viewing aggregator/categories/* pages.
2) *Should also test expired items* Appropriate aggregator feeds periodically refreshing.
3) *DONE* The aggregator module's provided blocks.
4) *OPEN* aggregator_element_data().
5) *adressed by #364467: Remove-items followed by update items does not work* aggregator_refresh() with specified etag and modified.
6) *OPEN* Update with no new syndicated content.
7) *OPEN* Update to a moved feed.
8) *OPEN* Make sure lines 607-609 use theme('image') instead of other stupid stuff.
9) *OPEN* Update to unparsable feeds.
10) *OPEN* aggregator_parse_w3cdtf()
11) *OPEN* Trying to use aggregator_parse_feed() to parse invalid xml.
12) *OPEN* Update a feed with more than one new item.
13) *OPEN* Attempt to update to a feed with a duplicate item.
14) *DONE* Update to a feed in a category.
15) *DONE* View feed items in a category.
- In regards to 6), 7) and 9) there should be separate tests for these cases.
- We shouldn't remove items from the list only because they moved to different .module files.
- Could you lay down which cases you're going to address and create a follow up issue for those which won't be addressed by this patch? (Of course I'd encourage you to address all of them ;-)
Comment #13
BerdirI agree that we should not remove items and I also think that more functions have been added since these lists have been created. There are already issues for other files, I'm going to create new follow-ups for the new files.
Looking at http://drupal.org/files/issues/aggregator_coverage.png, the following files need more tests:
Existing: (Several of the functions in those lists are also tested in this patch, because they use the api functions that are in aggregator.module)
#276499: Tests needed: aggregator.pages.inc
#276486: Tests needed: aggregator.admin.inc
New (I will post lists of tested functions when I'm starting with the tests there)
#456390: Tests needed: aggregator.fetcher.inc
#456402: Tests needed: aggregator.parser.inc
#456408: Tests needed: aggregator.processor.inc
Comment #14
Dries CreditAttribution: Dries commentedI'm assigning this issue to alex_b. I'll commit it when alex_b marks it RTBC, and I'll let alex_b drive the process (if he wants to).
Comment #15
alex_b CreditAttribution: alex_b commented#14: OK
#13: I moved all open items from #12 to the new issues that you opened.
Which leaves us with:
1) *DONE* Viewing aggregator/categories/* pages.
2) *NEEDS WORK* Appropriate aggregator feeds periodically refreshing.
3) *DONE* The aggregator module's provided blocks.
14) *NEEDS WORK* Update to a feed in a category.
15) *DONE* View feed items in a category.
Am I missing one here?
The *NEEDS WORK* above indicate that:
2) testCron() should also test for expired items (as indicated in #12) and at a second look, I'd say it should also test whether the feed produced the expected numbers of feed items.
14) The association between feed and category shouldn't be created through a query (see db_insert() statement) but rather through the UI.
As I reviewed the patch, I did some minor code style adjustments here and there, mostly to comments and messages.
Comment #16
mustafau CreditAttribution: mustafau commentedReroll.
Comment #18
BerdirFinally :)
Added both.
Changed.
Comment #20
BerdirI shall not change the tests before making the patch...
Comment #22
Berdircron handling changed as it does now use DrupalQueue.
Updated the test case to call drupal_cron_run() instead of aggregator_cron() so that the queues are actually executed.
Comment #24
mustafau CreditAttribution: mustafau commentedRemoving accidental code coverage parts from the patch.
Comment #26
mustafau CreditAttribution: mustafau commentedComment #27
grndlvl CreditAttribution: grndlvl commentedThese two don't check that the block was added to the correct region and that is not that necessary to figure out if they were added to the correct region. The other checks right after should be sufficient in determining if the block was enabled.
I left in tests just in case.
I am not aware of this feature so I am not sure what this is doing. It also needs additional comments.
I have added some additional comments.
All looks good. Except the above items mentioned.
Thanks,
Jonathan
I'm on crack. Are you, too?
Comment #28
sun.core CreditAttribution: sun.core commentedSorry, but tests don't qualify as critical anymore.
Comment #29
aspilicious CreditAttribution: aspilicious commentedCan someone review this?
Comment #30
cwgordon7 CreditAttribution: cwgordon7 commented#27: 276493-27_aggregator_tests.patch queued for re-testing. (This was last tested over a year ago and I would be surprised if it still applied, let alone passed. I'm not even sure if this is necessary anymore).
Comment #31
retester2010 CreditAttribution: retester2010 commentedtrailing white space
40 critical left. Go review some!
Comment #32
Jody LynnComment #33
webchick.
Comment #34
BerdirWondering why this has been pushed to 8.x :) What's the policy for test additions?
I see no reason to not add them to a stable version. If nothing else, it helps to increase stability and it's doesn't count as an API change for me.
Sure, it should be added to 8.x first and then backported, but there is no 8.x branch just yet :)
Comment #35
Jody LynnComment #36
Rodgey CreditAttribution: Rodgey commentedThis bug might add some light to the case:
reproducing the bug:
1 - create more than 1 catagory in the feed aggregator.
2 - add a feed item (you might use f.e. "http://online.wsj.com/xml/rss/3_7085.xml")
!important: check more than one catagory.
3 - run update for this feed
bug 1:
You will now find out that the items are just catagorized for one catagory instead of the multiple catagories that you've put with a checkbox while adding the feed.
4 - click on the feed name in the feed overview
5 - choose catagorize
6 - add the checkboxes (again) for multiple catagories
Bug 2
Now the rss feed will be shown in multiple catagories, but cron won't run anymore and produce the following error:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1-281' for key 1: UPDATE {aggregator_category_item} SET cid=:db_update_placeholder_0 WHERE ( (iid = :db_condition_placeholder_0) ); Array ( [:db_update_placeholder_0] => 1 [:db_condition_placeholder_0] => 281 ) in aggregator_save_item() (line 170 of /***/aggregator/aggregator.processor.inc).
I've been experimenting a little and found that the index data from aggregator_feed_item in the db are blocking cron to run.
Comment #37
BerdirI am able to reproduce the first bug reported in #36, and updated the tests to account for this.
Also did some other refactoring, removed whole test class that is already covered by another one and added a createCategory() helper function.
The tests will fail, will upload a patch that fixes it here: #1023190: Wrong merge query in aggregator_save_item()
Comment #39
BerdirComment #40
bfroehle CreditAttribution: bfroehle commented#37: 276493-28_aggregator_tests.patch queued for re-testing.
Comment #41
BerdirYay, now they pass ;)
Comment #42
aspilicious CreditAttribution: aspilicious commentedCan someone review this test case?
Comment #43
bfroehle CreditAttribution: bfroehle commentedAt a minimum there are a few whitespace mistakes.
Comment #44
BerdirRe-rolled with the trailing whitespaces removed.
This patch will never be perfect, there is stuff that still isn't tested and there is a lot of old code in aggregator.test that can't all be cleaned up.
So my suggestion would be to try to get this commited rather sooner than later, especially so that it can be added to D7 too. Who knows what will happen with aggregator.module in D8...
Comment #45
BerdirUhm, with patch this time.
Comment #46
Lars Toomre CreditAttribution: Lars Toomre commentedI thought that I could help review this patch to speed it along, but I am unable to find where on drupal.org the current reference file aggregator.test is stored. Could someone possibly post the URL of the reference file?
In looking at the entirely new TestCron function, I had a couple of thoughts:
1) After adding an old feed item, I think that we should assert that the data base now contains one feed item. In essence, demonstrate started at zero, incremented by one and then cron went through and cleaned up the old feed item returning to zero records in the data base.
2) I did not understand where the 'cid' value of 19 comes from in aggregator_category_item. An inline comment would be appropriate here.
3) I was puzzled where the value of 5 came from in the '5 new feed items' test came from. I am guessing that it is the number of items in the default feed. If that is the case, why not use $this->getDefaultFeedItemCount() instead of a hard coded value of 5.
4) Generally I think it is better to gather/compute the value for the test and then do the assertion test. Hence, I would reorder the code lines at the bottom of TestCron().
5) Based upon the problems I have seen with the aggregator feed in cron, I would check for two further conditions in this test subroutine.
5a) After the five feed items have been added, I would suggest that we mimic the passage of time and it is now time for another cron run. Run cron and attempt to retrieve/process the same feed. There should still be 5 feed items in the data base when this completes, confirming that no duplicates were added. I believe the TIMESTAMPs for these items should remain equal to REQUEST_TIME.
5b) After successfully finishing the 5a) test, I would manually delete one of the middle records, like the third feed record for instance. I would then simulate the passage of time and do another cron run to confirm that the one 'missing' feed item has been added back to the data bases with the new simulated time.
With the above changes, I think we can affirmatively confirm that cron can create Create/Update/Delete feed items with aged, new and duplicate feed items.
The one further problem that I have encountered deals with the case when a node body has been updated for clarification or to change a spelling error sometime after the node is published and included in the original rss.xml. I would hope that the feed item in the aggregator data base would be updated in a cron job for this small spelling change. Does it make sense to add this case as an assertion test in TestCron() as well?
Comment #47
Eric_A CreditAttribution: Eric_A commentedCould someone possibly post the URL of the reference file?
http://drupalcode.org/project/drupal.git/blob/HEAD:/modules/aggregator/a...
Use the "raw"-hyperlink if you need plain code.
Comment #48
Lars Toomre CreditAttribution: Lars Toomre commentedThank you Eric_A. The new git system has me confused when just wanting to review raw source code.
Comment #49
Berdir#45: 276493-29_aggregator_tests.patch queued for re-testing.
Comment #51
EclipseGc CreditAttribution: EclipseGc commentedAlso, check to make sure that all the potential pseudo-dependencies are enabled, as block deletion is part of the logic here, but it's often wrapped in a module_exists check and so code that should have testing coverage doesn't get fired.
We ran into this with Blocks as Plugins. Aggregator had direct calls to delete blocks from the blocks table, which we abandoned months ago, but never got errors on out of aggregator tests. I just found it as we were doing a last pass of manual testing.
Eclipse
Comment #52
Pat Redmond CreditAttribution: Pat Redmond commentedWorking on tests at Drupalcon Sydney 2013
Comment #53
Pat Redmond CreditAttribution: Pat Redmond commentedThe recent PSR-0 change means that the test coverage in the patches above don't apply correctly (since tests have moved in the directory structure) so this patch starts the process of creating tests that adhere to PSR-0.
Based on the test coverage outlined in the issue summary, the attached patch addresses the following:
1) Viewing aggregator/categories/* pages.
The following items have been addressed at another stage:
The following items require further review (they might be already included in the latest 8.x branch but they also may need to be be implemented):
2) Appropriate aggregator feeds periodically refreshing.
3) The aggregator module's provided blocks.
4) aggregator_element_data().
5) aggregator_refresh() with specified etag and modified.
6) Update with no new syndicated content.
7) Update to a moved feed.
8) Make sure lines 607-609 use theme('image') instead of other stupid stuff.
9) Update to unparsable feeds.
10) aggregator_parse_w3cdtf()
11) Trying to use aggregator_parse_feed() to parse invalid xml.
12) Update a feed with more than one new item.
13) Attempt to update to a feed with a duplicate item.
14) Update to a feed in a category.
15) View feed items in a category.
Comment #53.0
Pat Redmond CreditAttribution: Pat Redmond commentedNoting that the first test required has been addressed by post 52
Comment #53.1
Pat Redmond CreditAttribution: Pat Redmond commentedUpdated issue summary to reflect that there is a test which checks that cron runs and updates aggregator feeds correctly
Comment #53.2
Pat Redmond CreditAttribution: Pat Redmond commentedUpdated issue summary to reflect that there is a test which checks that the aggregator blocks are working
Comment #53.3
Pat Redmond CreditAttribution: Pat Redmond commentedUpdated issue summary- fix typo
Comment #54.0
(not verified) CreditAttribution: commentedUpdated issue summary to show the files providing the functionality
Comment #56
lokapujyaMissing test coverage isn't a bug, right?
The last patch is no longer relevant since #2127725: Remove category handling from aggregator
Keeping some comments from #45.
Comment #67
quietone CreditAttribution: quietone at PreviousNext commentedThe
aggregator
module has been removed from Core in10.0.x-dev
and now lives on as a contrib module. Issues in the Core queue about theaggregator
module, like this one, have been moved to the contrib module queue.Comment #68
larowlanWe're keeping the lights on here now aggregator is out of core.
I think at this point we're likely to only add new tests for new bugs/features.
Given the age of this patch, going to close as outdated.
Thanks everyone for their efforts to this point.