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.
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.
Comment | File | Size | Author |
---|---|---|---|
#25 | 2510230-datalayers-25.patch | 917 bytes | tim.plunkett |
#23 | 2510230-datalayers-23.patch | 454 bytes | tim.plunkett |
#17 | datalayer-tests-2510230-17-D7.patch | 707 bytes | bighappyface |
#14 | datalayer-tests-2510230-14-D7.patch | 553 bytes | bighappyface |
#12 | 2510230-datalayers-12.patch | 431 bytes | tim.plunkett |
Comments
Comment #1
doublejosh CreditAttribution: doublejosh as a volunteer commentedQuick 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.
Comment #2
bighappyface CreditAttribution: bighappyface as a volunteer commentedThank 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
Comment #3
bighappyface CreditAttribution: bighappyface as a volunteer commentedComment #4
bighappyface CreditAttribution: bighappyface as a volunteer commented@doublejosh any chance to review this yet?
Comment #5
pobster CreditAttribution: pobster as a volunteer commentedI'm liking this approach, very neat! +1
Comment #6
bighappyface CreditAttribution: bighappyface as a volunteer commentedHere is another patch without the code style adjustments. I'll submit them in another issue.
Comment #7
tim.plunkettI ran the tests locally:
(Sidenote, this project should have the issue queue integration turned on for automated tests, so patches will run the test suite)
Comment #8
doublejosh CreditAttribution: doublejosh as a volunteer commentedThanks! Having a look.
Comment #10
doublejosh CreditAttribution: doublejosh as a volunteer commentedTest result after committing patch...
https://qa.drupal.org/pifr/test/1094988
Comment #11
tim.plunkettI don't see how that's possible, that function is in datalayer.module itself...
Comment #12
tim.plunkettMaybe this?
Comment #14
bighappyface CreditAttribution: bighappyface as a volunteer commentedTrying another method for enabling the module. The tests all pass locally for me as well, through Drush and run-tests.sh
Comment #15
tim.plunkettComment #17
bighappyface CreditAttribution: bighappyface as a volunteer commentedPerhaps a check and include?
Comment #18
tim.plunkettComment #20
tim.plunkettOkay so that fails for completely other reasons, aka the function is found now.
#12 should have worked, not sure why it doesn't.
Comment #21
bighappyface CreditAttribution: bighappyface as a volunteer commented@tim.plunkett the `DrupalUnitTestCase::setUp`method doesn't make use of `func_get_args`as does `DrupalWebTestCase::setUp`, no?
Comment #23
tim.plunkettGood point.
Borrowing this from core's SearchExpressionInsertExtractTestCase.
Comment #25
tim.plunkettThis "fixes" it, but I don't know if that's correct.
Comment #26
bighappyface CreditAttribution: bighappyface as a volunteer commented+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!
Comment #31
tim.plunkettBranch tests are back to passing. Thanks!
https://qa.drupal.org/pifr/test/1094988
Comment #32
doublejosh CreditAttribution: doublejosh as a volunteer commentedCool. I'll put out a new release shortly.