I discovered this while trying to fix the image module test...

Part of image module's test is to create a new image node type. Currently, it's doing this by manually building some properties and then doing a post request to node/add/image. However, there's no reason it shouldn't use drupalCreateNode() for this, so that it's consistent with other modules, and we don't have to duplicate the tests to check whether the node was created successfully or not -- that ought to be drupalCreateNode()'s job, where the image module test can focus purely on the image-specific validation.

However, with the way drupalCreateNode() currently works, there appears to be no way to attach a file to a node. This is because it's running through node_save() directly, and /not/ through the form submission process an end-user would go through. So it's bypassing all of this extra validation/submission handling.

I tried simply changing node_save() to drupalPostRequest('node/add/$type' ...); but that causes problems with some of the workflow settings, etc. Still working on a better fix...

I've bumped this up to critical, as this is a critical API function that must be working properly.

Comments

beeradb’s picture

Status: Active » Needs review
FileSize
2.41 KB
1.03 KB

I didn't get as far on this as I had hoped, but i'm whiped out for the day. attached is a patch for the drupalCreateNode function, as well as an updated version of page_creation.test updated to use drupalCreateNode(). drupalCreateNode() will now only supply a title, and perhaps a body depending on the content type. All other parameters must be supplied in the settings array by the test. The test is also responsible for making sure the user account has all appropriate permissions to set the values in the settings array(naturally).

I didn't have time to redo the image_module.test and check against that patch to verify uploads, it would be great if someone could test on that.

If anyone cares to give some feedback on this, i'll pick up the fight again tomorrow if I need to.

-b

beeradb’s picture

Title: drupalCreateNode does not allow for file uploads » drupalCreateNode does not use the internal browser
FileSize
6.67 KB
3.11 KB

Heres some more work in this department. Heres what I got:

node_tests.patch
1). Altered story_edit.test to conform to coding standards. Also updated it to use additional permissions required by drupalCreateNode.
2). page_view.test- updated for new permissions
3). Altered node_revisions to move towards working with the new drupalCreateNode. I think the remaining problems are centered in the method prepareRevisions, which is trying to pass a nid into the drupalCreateNode() method. I'm not really sure why it's doing this, comments would be appreciated.

drupal_test_case.php.patch
1). Updates to method drupalCreateNode to use the internal browser.
2). Added a drupalLogoutUser() function. I'm not sure if it should be bundled here, but it made sense when working with the node revisions prepareRevisions, since that is called by each test and needs to have different permissions. It seems cleaner to log a user in, perform the requested actions, log the user out, and pass the results back to the tests for their usage. I also think it's a method we need anyway.

Any and all comments appreciated.

Rok Žlender’s picture

Node_save was used b/c it is much faster (you don't need to login and create user you can use your user, and you don't need 2 page loads for node creation itself). As SimpleTest is pretty slow I was looking for fastest solution. But if we are lacking in functionality as it seems we are here we need to reconsider.
Is there any other way to upload file/files to node without calling drupalPost()?

node_tests.patch
re 3) we were sending nid to drupalCreateNode so we were sure that new revision will be created on an existing node

drupal_test_case.php.patch
re 2) I agree drupalLogoutUser is needed maybe we could rename it to drupalLogout and also rename drupalLoginUser to drupalLogin, again I don't like the extra drupalGet. Can we assume that when logging out you land on homepage and that there should be no more 'Logout' link? Would this be enough?

beeradb’s picture

Rok,

It sounds like your feedback to #3 indicates we should add a drupalUpdateNode() which uses the post as well. My personal thoughts are that performance is secondary to accuracy on this stuff, and in general we should push towards using the methods end users would be using wherever possible. It might be worth it to look into a suite of node creation utilities, such as drupalCreateNode, drupalUpdateNode, and perhaps a function which just uses node_save, for times when node creation type has no effect on the outcome of the test. Thoughts?

As far as the drupalLogoutUser feedback goes, i'm all for renaming those functions to drupalLogin and drupalLogout, I found the name particularly verbose, but used it to maintain consistency. My concern with just detecting the "login" link on the main page revolves around people using simpletest during site development, it's a pretty normal use case to not have a login link on the front page of a site, particularly one that doesn't invite public participation. Is there anywhere we can meet in the middle on this? perhaps something in the browser session? Any thoughts you have on this would be great, if not i'll do some research when I get time and can pick up the fight on this issue (probably this weekend or next week). I also plan on adding some bells and whistles to the "breakup tests by method" patch I submitted, I think we can do some fun things with that.

boombatower’s picture

Status: Needs review » Needs work

As for renaming drupalLoginUser to drupalLogin I think that is a good idea. I also thing a function for logout would be a plus for consistency.

Not sure what you are referring to by the login link. I think Rok said check for no "logout link".

Two notes on the drupalCreateNode function.

  • There should probably be two different sets of function (like you said) one that uses drupalPost and one that doesn't.
  • As for attaching (or performing any additional operations) to a node you can simple edit the created node and submit it with the values you wish. I still agreed that two different sets of functions should be created.

Lastly the image test no longer exists in SimpleTest since the image module isn't part of core. The tests and any issues should be placed in the image module queue.

boombatower’s picture

Version: 6.x-1.x-dev »
Status: Needs work » Needs review
FileSize
3.81 KB

I have created a patch which adds a parameter to the drupalCreateNode function that will use the browser if set to true.

webchick’s picture

Hm. I'm not crazy about this suggested fix. I'd rather explicitly call whatever hook needs to happen to make file uploads possible, in addition to node_save(). Not having this is probably going to cause failures in other unexpected places as well, and passing TRUE as a second argument is not an intuitive way to solve it.

boombatower’s picture

I'm not sure we want to have it call the hooks needed to upload a file since that is just one possible extra value needed. As noted above:

As for attaching (or performing any additional operations) to a node you can simple edit the created node and submit it with the values you wish. I still agreed that two different sets of functions should be created.

I think that is the best solution.

The main reason for using the browser is to test the browser interface.

boombatower’s picture

This has created the issue: http://drupal.org/node/239560.

beeradb’s picture

Alright, I think it's clear we need this functionality, and it's actually not a difficult change(barring perhaps some complications with file uploads, which seem to _always_ be the exception to this stuff).

So the question is, what should we name them?

drupalCreateNode()
drupalCreateUserOwnedNode()??!?

the latter seems exceptionally wordy, but offhand I'm having difficulty coming up with function names which denote the meaning without being overly verbose.

thoughts?

boombatower’s picture

I think that drupalCreateNode should be assumed to create a node for current user that is logged in. This would be consistent with everything else in the test that is done.

I vote for changing drupalCreateNode to use the internal browser for complete consistency.

webchick’s picture

I think using the internal browser is also the simplest approach. This type of helper function is really only going to be used in functional tests anyway, I think...

beeradb’s picture

I agree that the current drupalCreateNode needs to be changed to use the internal browser. However I still agree with previous conversation to the point that we should have additional helper functions which just create and spit out content in the fastest way possible (node_save) for when the simpletest user owning the node isn't required(or perhaps even essential that they don't!). So the question is, are there are other node cereation functions we need (internal browser being creating node from another user?), or does this get them all? and, what do we rename what is currently drupalCreateNode to?

boombatower’s picture

@beeradb: If you want to create a node for a specific user you login as that user. It may be faster, but the tests aren't intended to be performance benchmarks, but instead to test the functionality. I think that having it user the browser makes it consistent since everything else done through the test is related to the user that is logged in (or none if not logged in).

To make it consistent it needs to use the internal browser. The only remaining question is to the relevancy of keeping the old function around.

beeradb’s picture

Assigned: Unassigned » beeradb
FileSize
7.79 KB

Here we (finally) are. This patch has a number of changes.

They are:
1). Move drupalCreateNode to drupalCreateNodeInterally($values). This does things the old fashioned way and uses node_save.
2). Creates a drupalCreateNode($values) which uses the browser to do all it's handy work.
3). Creates a drupalUpdateNode($values) which allows you to update existing content. Originally I wanted to make this just pass the node in, but that gets rather problematic with drupalPost returning a ton of errors for not being able to set fields. Currently it will automatically change the title/body if you don't specify them in the $values array. I'm thinking this may need to change and just use a passed in node object, calling drupalPost with the $tamper flag set to true. Thoughts?
4). Updates the node tests to use the new method. More test pass now than when I started, and the 2 failures i'm getting appear to be false negatives, as when printing the contents of a drupalGet on the pages, the text i'm asserting for is present.

boombatower’s picture

Status: Needs review » Reviewed & tested by the community

Looks really nice.

I like the drupalUpdateNode method and the renaming of drupalCreateNode to drupalCreateNodeInternally.

The work on node tests seems to fix a number of issues.

I tested a few other tests that used to pass and they still do, which is a good sign.

My tamper flag do you mean so that it will read attributes on object or what are you referring to?

I'll hold off committing until we get some more feedback.

beeradb’s picture

i'm honestly surprised other tests would pass, although I didn't try any of them. I expected the change in permissions needed when calling drupalCreateUser before creating nodes would make most of them break pretty badly. It's good to hear it's not as bad as I expected. Even things that are broken due to this patch should be fixed really easily by searching for drupalCreateNode calls and then making sure the simpletest user has correct permissions(as long as they should). The node test was slightly more difficult due to the need for revision testing, hence the arrival of drupalUpdateNode.

By the $tamper flag I mean that drupalPost has $tamper parameter which is described as such:
* @param $tamper
* If this is set to TRUE then you can post anything, otherwise hidden and
* nonexistent fields are not posted.

Currently the logic built around that is just a stub, with just a todo noted inside it. Upon thinking further I that's actually most likely there to test security related things, i.e. values set with #value on a form shouldn't be able to be altered regardless of what comes over post.

My personal feelings are even if we can just pass drupalUpdateNode() the node object itself, it's really messy to be posting all the crap anyway, and we should only be posting what we want updated. It would be very nice to just be able to say $this->drupalUpdateNode($node) though. Perhaps drupalUpdateNode should have a set of fields which it automatically strips out? I'm not terribly passionate about this either way, just throwing out what I see as our options.

boombatower’s picture

Like I said I tested the patch on a few tests so still not sure if this has wider reaching effects. Definitely something I will look into further.

As for the tamper, that would definitely be useful for testing form API and such, not sure how it relates to drupalUpdateNode since that should only be posting to fields that exists. Maybe I misunderstood your reason for bringing it up.

As for the drupalUpdateNode a route that I will suggest is to just pass the array of changes you wish to make and the node object. Then have the function return the updated node object, and post the necessary values.

On another note, every field that is displayed on the page is POSTed anyway, so probably not an issue if drupalUpdateNode just sets them all.

Just my two cents.

boombatower’s picture

Status: Reviewed & tested by the community » Needs work

drupalUpdateNode either needs the docs updated to say explicitly nid is required or move the nid to a separate parameter. i prefer the latter.

-chx

Other than that I think this is ready to go, so without further ado this will be committed once this has been fixed.

boombatower’s picture

Status: Needs work » Needs review
FileSize
5.45 KB

This is a rerolled the patch.

We still need to merge the last 3 comments and figure out what we want.

beeradb’s picture

FileSize
5.51 KB

here we go, the nid is now broken out into a new param.

boombatower’s picture

Project: SimpleTest » Drupal core
Version: » 7.x-dev
Component: Code » simpletest.module

Moving.

ksenzee’s picture

FileSize
5.46 KB

I had to apply this by hand now that simpletest is in core. Here's a reroll that applies to the latest HEAD. I compared the two patches and I'm pretty sure they're identical. It breaks about a kajillion tests but I assume that's a feature, not a bug...?

boombatower’s picture

Status: Needs review » Needs work

I had the same issues when I first tried it.

I didn't think it would do that much damage, but it definitely needs to be accompanied by a patch that fixes the tests. That was my original reason for not committing this.

boombatower’s picture

Status: Needs work » Postponed

Once we have all core tests passing I will apply this and make sure that it doesn't break any tests (as it did before) and fix any issues.

moshe weitzman’s picture

slow down. we absoluetely need a fast way to create nodes like whats there now. simpletest has to get faster, not slower. we want to avoid using the browser except when thats what is being tested.

for example, lets say that we are testing a 'send node via email' feature. IMO< that test should create a node without the browser. node_save() is fine, or drupal_execute(). and then it should use the browser to click the send link and fillout the form and so on. but tests that aren't about node creation should not pay a performance penalty.

so, -1 for this.

boombatower’s picture

I think we should have both functions or a flag to choose the method.

boombatower’s picture

bdragon’s picture

Subscribing.

Two things, as far as I can tell (testing on drupal 6):

A) Need to convert $edit into an associative array before calling drupalPost().
Here's what I use in my tests to do the same thing:

  /**
   * Flatten a post settings array because drupalPost isn't smart enough to.
   */
  function flattenPostData(&$edit) {
    do {
      $edit_flattened = TRUE;
      foreach ($edit as $k => $v) {
        if (is_array($v)) {
          $edit_flattened = FALSE;
          foreach ($v as $kk => $vv) {
            $edit["{$k}[{$kk}]"] = $vv;
          }
          unset($edit[$k]);
        }
      }
    } while (!$edit_flattened);
  }

B) The url needs to have _ replaced by - (Again, not sure if D7 needs this or not, testing on D6)

    $this->drupalPost('node/add/'. str_replace(array('_', ' '), '-', $type), $edit, t('Save'));
beeradb’s picture

Assigned: beeradb » Unassigned
Dave Reid’s picture

Priority: Critical » Normal

It's not that hard to do the in-browser node creating in the tests yourself. I doubt this is critical if it doesn't make it. If anything we'll have the 7.x-2.x version to test this.

boombatower’s picture

Status: Needs work » Closed (won't fix)

Seems appropriate.