Move database assumptions from field.test to field_sql_module.test

I would like to work on this issue. Question shouldn't they be converted to use the field storage API so all field tests work with all storage modules?

Thanks for the response. I i'll get started.

Had some problems

In file simpletest\tests\field_test.module
1. Didn’t know how to convert function field_test_entity_save.
2. Didn’t know how to convert function field_test_entity_load.

In file field\field.test
Line 997 (after patch) thought it was ok to delete line but wasn’t sure, so I commented it out
In file field\modules\text\text.test
Line 159 not sure how to covert .

needs automatic testing

I agree that this can probably go away :

+    // Check that a table has been created for the field. 
+    //TODO: THIS TEST IS INCORRECT fields do not require an sql database
+//    $this->assertTrue(db_table_exists('field_data_' . $field_definition['field_name']), t('A table has been created for the field.'));

This query is not related to Field storage, and shouldn't be altered by this patch :

     // Allow the user to use the 'Full HTML' format.
+    //  TODO: remove this db_update
     db_update('filter_format')->fields(array('roles' => ',2,'))->condition('format', 2)->execute();

Likewise, there's no reason to change anything in field_test_entity_save() / field_test_entity_load(), the direct queries are run on the 'test_entity' primary table (the equivalent of the 'node' table for 'test_entity' objects), not on field storage tables.

Added old versions of
FieldTestAttachLoad(),
FieldTestInsertUpdate(),
FieldTestAttachSaveMissingData()
to Class FieldSqlStorageTestCase.
And thanks to yched's suggesions fixed problems I had in comment 4.

rerolled
reapplied
retested
ok

Great ! Here are my remarks so far:

1) Not sure about $this->test_data. It's only used in testFieldAttachISaveMissingData() so it could be a simple variable inside the test.

2) The many try { } catch constructs are not needed, and the obfuscate the code flow quite a bit. Only field_attach_validate() raises exceptions, and in this case they are actually the feature to test.

3) testFieldAttachLoad() : it seems the code to create revisions could go in a foreach loop ?

4) there are a few places where you do

- $values[$delta]['value'] = mt_rand(1, 127);
+ $values[$delta]['value'] = $delta;

Why is that ? Can we keep the random values ? (note that 0 should be avoided, because it's considered 'empty' by the field_test field type.

4) Actually, if we remove the direct db insertion of values, testFieldAttachLoad() becomes a test for the insert-or-update / load cycle, so I think testFieldAttachLoad() and testFieldAttachInsertAndUpdate() should be merged somehow

5) testFieldAttachISaveMissingData() : There's a 'I' that slipped in the func name :-)

Additionally, the patch also includes the changes in #368639: Remove drupalCreateField/Instance in favour of standard API functions. I'm well aware that maintaining two separate patches is a (reasonable) pain, but we should really try to make both patches independant if we want them committed.

That's it for a first pass of review. When point 2) is addressed, it should be easier to check that every test is preserved.

Side note from Barry on IRC : One acceptance criteria would be to run the tests with pbs module on and check there are no fails.
Requires pbs to be added to FieldAttachTestCase::setUp (which would obviously not make it in the final core patch)

1. Removed #368639 changes from this patch.
2. Removed the try, catches that I had added except for the ones around field_attach_update().
3. Merged testFieldAttachLoad() with testFieldAttachedInsertAndUpdate().
4. Put random values back in multiple value fields. Verified correct values were loade back..
5. Added foreach construct while building revisions.

Could not get the pbs module to run enabled.
After enabling it then selecting testing I get

* Warning: Missing argument 3 for pbs_field_attach_load(), called in C:\wamp\www\drupal_head\modules\field\field.attach.inc on line 343 and defined in pbs_field_attach_load() (line 196 of C:\wamp\www\drupal_head\sites\all\modules\custom\pbs\pbs.fieldapi.inc).
* Warning: Missing argument 4 for pbs_field_attach_load(), called in C:\wamp\www\drupal_head\modules\field\field.attach.inc on line 343 and defined in pbs_field_attach_load() (line 196 of C:\wamp\www\drupal_head\sites\all\modules\custom\pbs\pbs.fieldapi.inc).
* Notice: Undefined variable: age in pbs_field_attach_load() (line 198 of C:\wamp\www\drupal_head\sites\all\modules\custom\pbs\pbs.fieldapi.inc).

Ah, that's because pbs hasn't been updated to the final state of #368674: Implement hybrid storage for Fields in Core. OK, forget about it for now.
Setting to 'needs review' so that the test bot can chime in.

- 3 test failures in 'Field attach tests', says bot :-)

- FieldSqlStorageTestCase::setUp():
The instance definition can simply be

$this->instance = array('field_name' => $this->field_name, 'bundle' => 'test_bundle');

The other properties are used by some field_attach tests, where the code was copied from, but are not used by the sql storage tests.

- FieldSqlStorageTestCase::setUp():
Minor : Please move the $this->table and $this->revision_table definitions below the field_create_instance() call.

- FieldSqlStorageTestCase:
Missing whiteline between testEntityTypeId() and testFieldAttachLoad(), between testFieldAttachLoad() and testFieldAttachInsertAndUpdate()

- FieldSqlStorageTestCase:
Let's use the PHPdoc comment format (multiline /* instead of //) for comments above testFieldAttachSaveMissingData() (I know that comes from the current code). While we're at it, it would be cool to add similar short comments above the other tests in there.

- "Removed the try, catches that I had added except for the ones around field_attach_update()." - why is field_attach_update() special ? exceptions in there would be test failures, I don't think we need to specifically catch them ?

- FieldAttachTestCase::testFieldAttachSaveMissingData():
The new assert messages ('Missing field in insert ok', 'with data ok') don't seem up to standards. Can we keep the previous messages ?

- FieldAttachTestCase::testFieldAttachSaveMissingData():
The part below '// Add some real data' is confusing. Previous tests added just one value. I don't think we need to test multiple values here.
In this case, please rename $test_data to $value (consistency with oher tests), and define it just above the insert, not at the beginning of the test.

That's it for now, I haven't really delved into FieldAttachTestCase::testFieldAttachLoadAndInsertAndUpdate() yet :-)

1. Only change made not in response to suggestions in comment 15, is in function FieldAttachTestCase::testFieldAttachSaveMissingData(). I added a field_attach_load() after field_attach_insert() to confirm that no values were inserted.

2. I fixed all of the suggestions. I agree with all of them especially the try’s and catches. The reasons they were left for the field_attach_update are too tragic/comic to mention. One might say I had a brain fart.

Thanks for driving this home yched and puradata. I will review and commit this when yched (or barry or karen) marked it as RTBC. Keep up the good work.

1. Added unique values for each revision test. Also I added a current_revision variable to
keep track of the current revision while I am building the revisions.
2. I fixed the indenting problem.
3. Thanks for the mentoring

There are a variety of minor things such as:

+  // Let other modules act on deleting the bundle
+      // Create initial content revision 0 
+        // create revisions
... etc

All comments should start with a capital letter and end in a period.

 class FieldAttachTestCase extends DrupalWebTestCase {
+  function testFieldAttachLoadAndInsertAndUpdate() {
+  function testFieldAttachLoad() {
etc.

These and other functions/classes need a line of PHPDoc to summarize what they do.

+[SPACE][SPACE][SPACE][SPACE]
...
+    }[SPACE]
etc.

There are a few lines where trailing whitespace is introduced.

I realize to a large extent you're copy/pasting code around, but let's fix this as long as you're touching those lines and/or the ones immediately surrounding them.

On a related note:

+  function testFieldAttachLoadAndInsertAndUpdate() {

Seriously? :) This function name seems to imply that we really ought to break this up a bit more, no? Otherwise, could we use something more descriptive here of what we're actually testing?

+    for ($rev_index = 0; $rev_index < 3; $rev_index++) {

What is $rev_index? We don't abbreviate variable names. From looking at the code of field_test_create_stub_entity() it looks like this is supposed to be $revision_id or similar? Also, why 3? And why two loops? Can we add a comment here so it's understood what exactly the code is doing?

+      // Create initial content revision 0 

I don't quite parse that. What does 'initial content revision 0' mean? From the code, it looks like this is more 'If this is the first revision, insert the field; otherwise, update it.' but I'm not sure that's accurate, and I'm also not sure what our larger 'goal' is in these branches of code. In short, let's have better comments here.

+  /**
+   * testFieldAttachSaveMissingData tests insert and update with missing or invalid fields. For the
+   * most part, these tests pass by not crashing or causing exceptions.
+   *
+   */

No need to repeat the function name here, and PHPDoc comments should follow the format:

/**
 * 1 Line Summary, max of 80 chars.
 *
 * The rest of the explanation. Blah blah blah...
 */

Let's go with "Tests insert and update with missing or invalid fields." as the first line, clarify the bit about what exactly "for the most part, these tests pass by not crashing or causing exceptions." means on the "rest of explanation" line, and remove the extra blank * line.

In the setUp function of FieldSqlStorageTestCase, could we get a quick comment above the $this->field_name = ... etc. lines that just summarizes what the following lines do, for easy scanning?

+  /**
+      * testFieldAttachSaveMissingData tests insert and update with missing or invalid fields. For the
+      * most part, these tests pass by not crashing or causing exceptions.
+      *
+      */

Something here got real messed up with the formatting. :) Same comments apply as above in the other testFieldAttachSaveMissingData function.

Incidentally, I realize this kind of feedback is probably kind of annoying, but:
a) bjaspan and yched already did a wonderful job tech reviewing, so there's not much else for me to do there. :)
b) puradata is a fairly new core contributor (and clearly an awesome one, since he's taking on field API stuff!!), so I want to instill in him best practices on the nit-picky stuff *now* so that it feeds into future patches.
c) since so few people understand Field API, IMO it's critical that whenever we touch the code to do whatever we can to make it more understandable to the next soul who happens along.

So how about testFieldLifecycle or something, with the PHPDoc explaining that it adds, loads, and whatevers a field?

I am working on fixing the comments. I think that I used the ridiculous function name too be funny. The function does a lot and needs more comments. I wasn't going to rewrite just comment it and rename it. Webchick I really do appreciate the thoughtful feedback. As JFK used to say "That which doesn't kill me. Makes me stronger"

1. Changed function name from testFieldAttachLoadAndInsertAndUpdate back to its original name of testFieldAttachInsertAndUpdate. If anyone has a better name I can easily change it or they can make a patch. Remember that the function is only called by the simpletest system because its name starts with test. Added comments to it, both phpdoc and // styles.
2. Fixed document and my editor so it doesn’t get confused inside phpdoc style comments. It turns my editor was using a different font for phpdoc comments and literally could not count spaces correctly.
3. I installed doxygen and ran it on field.test and field_sql_storage.test. I think the functions I worked on for this iissue are documented but am looking forward to your comments for improvements. Because I ran coder on the files I made formatting changes through out the file, but It doesn’t qualify me to document the other many routines.
4. I still don’t know why my editor is adding ending whitespaces, but I realized I can do a simple regex search and detect and remove them before I created my patches.

- testFieldAttachInsertAndUpdate -> testFieldAttachSaveLoad ? We do test Load as well, in there.
- What is your editor ? Some editors / IDE have an option to automatically trim whitespaces on save, which come really handy (although you all the whitespaces left in by other people then get in your way, but heh...)

I am using notepad++ and phpedit in a windows xp world. Notepadd++ is very fast phpedit uses xdebug for a debugger which is good. I'll look up the trim ending whitespaces. Thanks for the tip. As far as the function name I'll leave it up you people with more drupal experience than me. Wait I think I got one testFieldAttachLoadandInsertAndUpadteNameWrittenByANewbeeBecauseTheOldNameWastestFieldInsertAndUpdateBeforeItWasMergedWithtestFieldAttachLoad.
It has a certain rhythm to it.

testFieldAttachLoadandInsertAndUpadteNameWrittenByANewbeeBecauseTheOldNameWastestFieldInsertAndUpdateBeforeItWasMergedWithtestFieldAttachLoad

That name sounds like it was translated from Old Entish. :)

---

(Skimming over the code I can find no issues, but I'm very tired and shouldn't be relied on.)

This patch does no longer apply and needs a re-roll. Sorry!

Committed to CVS HEAD. Thanks all!

Followup patch :
- fixes a few missing whitelines
- makes it clear that the same function tests field_attach_insert(), field_attach_update() and field_attach_load()
- streamlines a few overly detailed explanations.

Plus we forgot that assert messages are supposed to be wrapped in t(). The rest of the field tests need to be fixed for this too, but getting the Save/Load test fixed ASAP will let us move forward more easily on #362024: Field Attach: Make hook_field_load() be 'multiple' like field_attach_load().

Thanks for the clean-up. Committed.

I have a patch for the remaining t() additions. Do you want me to open an issue for adding t() to translatable strings is field.test and field_sql_storage.test? Then submit my new patch to that issue.

Actually, I think we should probably start another issue to discuss whether to remove them entirely, if there isn't one already.

er. them == t()s around assertions, since that wasn't very clear.

webchick: LOL :-). So what should we do in between ? Like, for patches introducing new tests ? t() or no t() ?

yched: For now, we do what core does, which is t() these strings. What we ultimately do depends on the outcome of said discussion.

So It's not clear whether the t() around assertions are even needed? Well if they are I have a patch. I will not submit an issue. Thanks for the info.