Problem/Motivation

This is a great module, well thought and inclusive of greater Google practices and functionality, but the code is untested. As I move forward with using this module for my application datalayer, it would be great to know the code is tested and proven stable.

Proposed resolution

Implement tests for current functionality and to support further development.

Remaining tasks

Remove the disabling test file extension, complete the tests already within, and add tests as necessary to get good coverage of current functionality.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

doublejosh’s picture

Quick update from current maintainer...

Test file originally sought to create node types and taxonomy content via SimpleTest to confirm correct details are output. This was a large burden and tests were never completed. Unit tests alone is much more likely to happen. Very willing to accept PHPUnit flavored tests, but no immediate plans to do so yet.

bighappyface’s picture

Thank you for the feedback @doublejosh, and thank you for the open invitation to submit some tests.

I have submitted a patch that is ready for review. As far as the core module code, I made very few changes and they were only related to code style. For the tests, I added two new files, one for unit tests and one for web test cases, and I think I have a achieved a decent amount of coverage this first pass. The module's use of the `drupal_static` variable cache allowed me to inject a lot of mock data to test the existing code and we should have adequate regression moving forward. For the web tests, I added two simple checks for the variable output and settings output to give us some basic coverage against a bootstrapped instance of Drupal.

To aid review I have forked the code on GitHub and opened a PR against it's master: https://github.com/bighappyface/datalayer/pull/2

Thanks again for the feedback and opportunity to contribute. This is my first Drupal patch and I am excited to learn more about this process.

Cheers,

David

bighappyface’s picture

Status: Active » Needs review
bighappyface’s picture

@doublejosh any chance to review this yet?

pobster’s picture

I'm liking this approach, very neat! +1

bighappyface’s picture

Here is another patch without the code style adjustments. I'll submit them in another issue.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I ran the tests locally:

DataLayer Unit Tests 15 passes, 0 fails, and 0 exceptions
DataLayer 24 passes, 0 fails, 0 exceptions, and 6 debug messages

Test run duration: 11 sec

(Sidenote, this project should have the issue queue integration turned on for automated tests, so patches will run the test suite)

doublejosh’s picture

Thanks! Having a look.

doublejosh’s picture

Test result after committing patch...
https://qa.drupal.org/pifr/test/1094988

tim.plunkett’s picture

I don't see how that's possible, that function is in datalayer.module itself...

tim.plunkett’s picture

FileSize
431 bytes

Maybe this?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2510230-datalayers-12.patch, failed testing.

bighappyface’s picture

Trying another method for enabling the module. The tests all pass locally for me as well, through Drush and run-tests.sh

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: datalayer-tests-2510230-14-D7.patch, failed testing.

bighappyface’s picture

Perhaps a check and include?

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: datalayer-tests-2510230-17-D7.patch, failed testing.

tim.plunkett’s picture

Okay so that fails for completely other reasons, aka the function is found now.
#12 should have worked, not sure why it doesn't.

bighappyface’s picture

@tim.plunkett the `DrupalUnitTestCase::setUp`method doesn't make use of `func_get_args`as does `DrupalWebTestCase::setUp`, no?

Status: Needs work » Needs review
tim.plunkett’s picture

FileSize
454 bytes

Good point.

Borrowing this from core's SearchExpressionInsertExtractTestCase.

Status: Needs review » Needs work

The last submitted patch, 23: 2510230-datalayers-23.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
917 bytes

This "fixes" it, but I don't know if that's correct.

bighappyface’s picture

+1

@tim.plunkett much cleaner with `drupal_load`. I was looking for a function like that, but didn't look hard enough. Thanks a ton for the help!

The last submitted patch, 2: datalayer-tests-2510230-0-D7.patch, failed testing.

The last submitted patch, 6: datalayer-tests-2510230-6-D7.patch, failed testing.

The last submitted patch, 17: datalayer-tests-2510230-17-D7.patch, failed testing.

tim.plunkett’s picture

Status: Needs review » Fixed

Branch tests are back to passing. Thanks!
https://qa.drupal.org/pifr/test/1094988

doublejosh’s picture

Cool. I'll put out a new release shortly.

Status: Fixed » Closed (fixed)

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