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.

Comments

lyricnz’s picture

Assigned: Unassigned » 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!

lyricnz’s picture

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.

lyricnz’s picture

StatusFileSize
new20.27 KB

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

"279 passes, 0 fails, and 0 exceptions"

alex_b’s picture

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.

lyricnz’s picture

StatusFileSize
new77.53 KB

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?

lyricnz’s picture

StatusFileSize
new85.61 KB

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)

alex_b’s picture

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.

lyricnz’s picture

StatusFileSize
new119.77 KB

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)

aron novak’s picture

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

aron novak’s picture

Status: Needs work » Needs review
StatusFileSize
new120.11 KB

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

alex_b’s picture

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"

lyricnz’s picture

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.

lyricnz’s picture

Status: Needs work » Needs review
StatusFileSize
new120.17 KB

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:

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

use

    @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

aron novak’s picture

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.

alex_b’s picture

Assigned: lyricnz » Unassigned

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

alex_b’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thank you!

Status: Fixed » Closed (fixed)

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