This patch adds test for variable_get, variable_set and variable_del to the bootstrap tests. Any comments/additions appreciated.

CommentFileSizeAuthor
#7 variabletest.patch2.42 KBrobloach
#6 variabletest.patch2.69 KBbeeradb
#4 variabletest.patch1.7 KBbeeradb
#2 variabletest.patch1.75 KBbeeradb
variabletest.patch1.7 KBrobloach

Comments

boombatower’s picture

boombatower commented
Component: simpletest.module » tests
beeradb’s picture

beeradb commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new variabletest.patch1.75 KB

Tests passed and the code looks sane.

I went ahead and added periods to the comments as per coding standards. Marking RTBC.

damien tournoud’s picture

damien tournoud commented
Status: Reviewed & tested by the community » Needs work

There is DrupalWebTestCase::assertEqual and DrupalWebTestCase::assertIdentical for asserting equality. Also, you can use DrupalWebTestCase::randomName to generate random patterns.

Also I'm not sure if we want to hit the database directly. I'll rather see an new menu callback created in the system_test mockup module for this (ie. modules/simpletests/tests/system_test.module).

beeradb’s picture

beeradb commented
StatusFileSize
new variabletest.patch1.7 KB

I went ahead and implimented Damiens suggestions RE: Random string generation and asserts. I didn't touch the additional menu item stuff, as I'm not sure I track what he's asking for there.

damien tournoud’s picture

damien tournoud commented

There is two things to test here:

* Does the variable storage works locally: ie. if I call variable_set, and then variable_get, do I get the stored value? if I call variable_del and then variable_get, does the value is invalidated?
* Does the variable storage works across requests: ie. if I call variable_set, does an other request that later calls variable_get will get the stored value?

To test the later, I suggest to implement a menu callback in system_test.module that does: echo variable_get('simpletest_bootstrap_variable_test'); return;, and use a drupalGet() to hit that page and check the returned value.

beeradb’s picture

beeradb commented
Status: Needs work » Needs review
StatusFileSize
new variabletest.patch2.69 KB

Here we go. I implemented the suggested changes regarding a menu item in system_test.module. Because of this I've gone ahead and removed the direct database query - my thinking is that if the value is persisting across requests we can safely assume it's being stored correctly in the database.

Marking CNR..

robloach’s picture

robloach
he/him
commented
StatusFileSize
new variabletest.patch2.42 KB

Very nicely done, I removed the system_test_variable_get function and stuck the call directly in the menu hook:

  $items['system-test/variable-get'] = array(
    'title' => 'Variable Get',
    'page callback' => 'variable_get',
    'page arguments' => array('simpletest_bootstrap_variable_test', NULL),
    'access arguments' => array('access content'),
    'type' => MENU_CALLBACK,
  );

This was the only change I made, and tests all pass. RTBC in my eyes, but another review would be good.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

The patch looks good, thanks beeradb.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

beeradb’s picture

beeradb commented

Rob built the house, I just did a little finishing work :)

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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