Write simpletests

alex_b - April 16, 2009 - 13:14
Project:Feed Element Mapper
Version:6.x-1.x-dev
Component:Code
Category:task
Priority:normal
Assigned:Unassigned
Status:closed
Description

We need to write simple tests. I've avoided this issue in the past because it involves testing against other modules. However, missing tests is an impediment for ongoing changes like #215979: Create a CCK date field mapper, #397682: Let mappers declare mapping targets or #433384: Make mapper node-independent .

These need to be integration tests that actually test the mapper with a feed against the integrated module.

#1

lyricnz - April 30, 2009 - 08:17
Assigned to:Anonymous» lyricnz

I've started working on this. I have a simple test that creates a feed node, configures some mapping, and consumes an XML (really more of an exercise than a test).

Next, I will create some CCK field mappings, to test individual cases. Sample feed data welcome!

#2

lyricnz - May 2, 2009 - 14:33

FYI, while writing these tests, I came across a strange behaviour, which turned out to be a bug: #451564: Using timestamp fields in the Date mapper empties created node.

#3

lyricnz - May 2, 2009 - 15:05

First draft of the tests for base API, CCK mapper, and start of date mapper.

"279 passes, 0 fails, and 0 exceptions"

AttachmentSize
feedapi_mapper_tests.tgz 20.27 KB

#4

alex_b - May 2, 2009 - 15:30
Status:active» needs work

That's a great start. Here's my first round of review. I know you're in the middle of this, so some of these points may be on your list anyway:

  • Let's move tests into their own group "FeedAPI Mapper"
  • Stick to doxygen formatting conventions http://drupal.org/node/1354
  • feedapi_mapper_test.php should be feedapi_mapper_test.inc - more in line with other Drupal modules
  • Some files don't contain $Id$

I think it's a great idea to keep the tests in separate files. This will keep things overlookable. I also like that you're including the feeds in the tests. We should do this for FeedAPI as well. At a later point we may want to push these test feeds into the FeedAPI module.

Nice work.

#5

lyricnz - May 2, 2009 - 16:05

I've done 1,3,4 per your suggestion, and removed the <type> items from the comments. Did you have specific concerns with the comments?

AttachmentSize
feedapi_mapper_tests.patch 77.53 KB

#6

lyricnz - May 3, 2009 - 05:40

Added tests for Link and Taxonomy
353 passes, 1 fail, and 0 exceptions
(the failure is due to #451926: FeedAPI reports "1 existing item(s) were updated" when consuming a brand-new feed)

AttachmentSize
feedapi_mapper_tests.patch 85.61 KB

#7

alex_b - May 4, 2009 - 14:53

Great work. I think the approach is solid.

Time for nit picking :)

1) string concatenation is off at some points - should be Drupal 6.x style. e. g. line 174 in feedapi_mapper_test.inc
2) remove all debug output
3) I'm assuming that calls to writeFile() will all go away, but if some stay, we should use file names that are prefixed with feedapi_mapper_test_
4) Clean up comments:
- add a break after all @param $variable and indent explanation by 2 spaces - e. g. like on FeedApiMapperTestCase::_createContentType()
- Make sure that @file comments aren't attached to class definitions like in feedapi_mapper_test.inc .
5) rename TODO to @todo (consistent with feedapi_mapper.module)

I'm going to have a look at #451926: FeedAPI reports "1 existing item(s) were updated" when consuming a brand-new feed now.

#8

lyricnz - May 4, 2009 - 15:33

1) Not fixed yet - ask me tomorrow :(
2) Done - including writeFile() calls, except for the two test-cases I'm actively working on
3) Done - added prefix to filenames
4) Done
5) Done

Added start of emimage testcase - WIP. Added an ical sample file.

438 passes, 1 fail, and 0 exceptions (excluding date mapper; error as described above)

AttachmentSize
feedapi_mapper_tests.patch 119.77 KB

#9

Aron Novak - June 25, 2009 - 13:29

I get lots of fails now, maybe because of FeedAPI changes, further testing is needed. For example createFeed() does not work.

#10

Aron Novak - June 26, 2009 - 09:29
Status:needs work» needs review

Re-rolled.
Mostly i needed to touch initialization parts, surely it's because of changes in feedapi and other modules.

AttachmentSize
435450_mapper_tests.patch 120.11 KB

#11

alex_b - June 26, 2009 - 13:46
Status:needs review» needs work

Lyricnz reviewed and came back to me with:

"I get repeatable http 500 executing the date mapper test; the other five are fine though"

#12

lyricnz - June 26, 2009 - 13:48

I get repeatable HTTP 500 error executing the Date mapper test, but the other 5 pass fine:

Overall results: 450 passes, 0 fails, and 0 exceptions
FeedAPI Mapper Link: 94 passes, 0 fails, and 0 exceptions
FeedAPI Mapper Embedded Image Field: 82 passes, 0 fails, and 0 exceptions
FeedAPI Mapper Content: 88 passes, 0 fails, and 0 exceptions
FeedAPI Mapper: 114 passes, 0 fails, and 0 exceptions
FeedAPI Mapper Taxonomy: 72 passes, 0 fails, and 0 exceptions

This is with SimpleTest 6.x-2.8. Looking deeper now.

#13

lyricnz - June 26, 2009 - 14:01
Status:needs work» needs review

The problem with the Date test is the same as #472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks, which isn't fixed in Core D7 yet.

You can work around the issue by declaring the order of your module requirements, without relying on the dependency system. Ie: instead of:

<?php
   
@parent::setUp(
     
'feedapi',
     
'feedapi_node',
     
'parser_simplepie',
     
'parser_common_syndication',
     
'feedapi_mapper',
     
'content',
     
'date',
     
'date_api',
     
'date_timezone'
   
);
?>

use

<?php
   
@parent::setUp(
     
'feedapi',
     
'feedapi_node',
     
'parser_simplepie',
     
'parser_common_syndication',
     
'feedapi_mapper',
     
'content',
     
'date_api',
     
'date_timezone',
     
'date'
   
);
?>

(Note the order of the last three modules). Reroll attached - this is the only change

AttachmentSize
mapper_tests.patch 120.17 KB

#14

Aron Novak - June 27, 2009 - 09:26
Status:needs review» reviewed & tested by the community

#13, really strange that this problem did not appear at my dev environment...
All in all, i tested it, it seems that now it's both fine in my and your environment, so now it's RTBC.

#15

alex_b - July 1, 2009 - 12:17
Assigned to:lyricnz» Anonymous

Yoohoo. Tests! Need to review & commit as soon as I get some time over here.

#16

alex_b - July 1, 2009 - 20:37
Status:reviewed & tested by the community» fixed

Committed. Thank you!

#17

System Message - July 15, 2009 - 20:40
Status:fixed» closed

Automatically closed -- issue fixed for 2 weeks with no activity.

 
 

Drupal is a registered trademark of Dries Buytaert.