Support from Acquia helps fund testing for Drupal Acquia logo

Comments

smccabe created an issue. See original summary.

smccabe’s picture

smccabe’s picture

missing newlines added

mglaman’s picture

+++ b/modules/line_item/tests/commerce_line_item.test
@@ -0,0 +1,41 @@
+    //simple test to check API functionality
+    debug(commerce_line_item_types());
+    $line_item_types = commerce_line_item_types();
+    $this->assertNotNull($line_item_types['product'], t('commerce_line_item_types() returned the correct result'));

Should we do a test to ensure callbacks generated properly?

joelpittet’s picture

Status: Needs review » Needs work

Driveby review. +1 to getting these tests started. I'd say just a tiny bit more and just commit it and work from there.

  1. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,41 @@
    +    //simple test to check API functionality
    
    +++ b/modules/payment/tests/commerce_payment.test
    @@ -0,0 +1,40 @@
    +    //simple test to confirm we are returning payment method titles
    

    Little nits: end with period, apt and space before first word.

  2. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,41 @@
    +    debug(commerce_line_item_types());
    

    Left over I presume?

  3. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,41 @@
    +    $this->assertNotNull($line_item_types['product'], t('commerce_line_item_types() returned the correct result'));
    

    Agree with @mglaman, we should check we are getting more than nonNull

liamanderson’s picture

I've integrated the suggested changes in comments #4 and #5. Can someone please check that the test for the callback is good?

The last submitted patch, 6: interdiff-2624700-3-4.diff, failed testing.

liamanderson’s picture

The last submitted patch, 8: interdiff-2624700-4-5.diff, failed testing.

smccabe’s picture

Test addition looks good to me, I wrote the first part so I probably can't RTBC, @joelpittet?

joelpittet’s picture

Status: Needs review » Needs work

A slightly more thorough review(yet still quick as hell):

  1. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,50 @@
    +    $modules = parent::setUpHelper('all');
    
    +++ b/modules/payment/tests/commerce_payment.test
    @@ -0,0 +1,40 @@
    +    $modules = parent::setUpHelper('all');
    

    Is all really needed or maybe we can speed this test up by slimming down to minimum until those other modules are needed?

  2. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,50 @@
    +  function testCommerceLineItemTypes() {
    
    +++ b/modules/payment/tests/commerce_payment.test
    @@ -0,0 +1,40 @@
    +  function testCommercePaymentTitles() {
    

    A docblock comment above the test functions to summarize what you are testing with these tests would help.

  3. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,50 @@
    +    // Simple test to check API functionality.
    

    This may be good enough for the docblock instead?

  4. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,50 @@
    +      $this->assertTrue(is_array($line_item_type['callbacks']), t('commerce_line_item_types() returned the correct type'));
    

    If I recall correctly, we shouldn't be translating assertions. @see the note on this page for reference: https://www.drupal.org/node/265828

  5. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,50 @@
    +        // Ex. $callback_action: 'configuration', 'title', 'add_form', 'add_form_submit'.
    

    nit: 80 chars wrap.

  6. +++ b/modules/line_item/tests/commerce_line_item.test
    @@ -0,0 +1,50 @@
    +  }
    +}
    

    Nit: end line break before ending class curly

liamanderson’s picture

I have updated the patch with the suggestions in comment #11 with the exception of #1. I am not sure what modules we could slim this down to.

I have also updated the patch name based on the suggestion from Dreditor.