Test needs to be defined for pmtimetracking.

CommentFileSizeAuthor
#14 pm-pmtimetracking-test-1975514-09-bypass.patch7.72 KBD34dMan
#14 pm-pmtimetracking-test-1975514-09.patch7.6 KBD34dMan
#7 pm-pmtimetracking-test-1975514-05.patch9.97 KBD34dMan
#1 pm-pmtimetracking-test-1975514.patch10.38 KBD34dMan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented
Status: Active » Needs review
FileSize
pm-pmtimetracking-test-1975514.patch10.38 KB

Status: Needs review » Needs work

The last submitted patch, pm-pmtimetracking-test-1975514.patch, failed testing.

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented
Status: Needs work » Needs review

YAY! I was looking forward for a it to fail. But the errors are due to missing organization id information on user field. So that would be another patch.

juliangb’s picture

juliangb CreditAttribution: juliangb commented

So if I understand correctly, these tests should pass after the organization id patch is committed?

A few comments:

+++ b/pmtimetracking/pmtimetracking.testundefined
@@ -14,41 +15,262 @@ class pmtimetrackingTestCase extends DrupalWebTestCase {
+    $nid = 1;

No need to set a nid, as core will do that for us, on an auto-increment.

+++ b/pmtimetracking/pmtimetracking.testundefined
@@ -14,41 +15,262 @@ class pmtimetrackingTestCase extends DrupalWebTestCase {
+      'param' => array(
+        'access content',
+        'create pmorganization content',
+        'Project Management Organization: add',
+        'Project Management Organization: view all',
+        'create pmperson content',
+        'Project Management Person: add',
+        'Project Management Person: view all',
+        'create pmproject content',
+        'Project Management Project: add',
+        'Project Management Project: view all',
+        'create pmtask content',
+        'Project Management Task: add',
+        'Project Management Task: view all',
+        'create pmticket content',
+        'Project Management Ticket: add',
+        'Project Management Ticket: view all',
+        'create pmtimetracking content',
+        'Project Management Timetracking: add',
+        'Project Management Timetracking: view all',

We need to find a way of hiding the permissions (such as 'create pmtimetracking') that core now adds for every new content type, or we use those instead of ours. Currently we have some duplicates that could confuse users.

In Drupal 6, these permissions did not exist, so we just used our own ones.

+++ b/pmtimetracking/pmtimetracking.testundefined
@@ -14,41 +15,262 @@ class pmtimetrackingTestCase extends DrupalWebTestCase {
+  function pmRunTest( &$test ){
+    foreach ($test as $k => $t) {
+      if ( isset($t['status']) AND $t['status'] == 'COMPLETED') {
+        continue;
+      }
+      // debug('Array contents: ' . var_export($t, true));
+      switch ($t['command']) {
+        case 'visit_page':
+          $this->drupalGet($t['param']);
+          break;
+        case 'content_add':
+          $this->drupalPost('node/add/' . $t['entity'], $t['param'], t('Save'));
+          break;
+        case 'user_create':
+          $user = $this->drupalCreateUser($t['param']);
+          $test[$k]['entity'] = $user;
+          break;
+        case 'user_login':
+          $this->drupalLogin($test[$t['param']]['entity']);
+          break;
+      }
+      $test[$k]['status'] = 'COMPLETED';
+    }

I wonder whether it is better to have this function or not. Firstly, if we did have it, wouldnt' it be better in pm.test so that it would be the same for all pm modules?

The reason I doubt whether we should have it though, is because by having our own way of writing tests we raise the barrier for other Drupal developers wanting to quickly understand the code. How much time / coding does it save?

juliangb’s picture

juliangb CreditAttribution: juliangb commented

I've just found the way of removing the default permissions.

http://api.drupal.org/api/drupal/modules%21node%21node.module/function/n...

I'll submit a patch for all pm content types, so the only change for this patch would be to remove the standard permissions from the test.

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented

So if I understand correctly, these tests should pass after the organization id patch is committed?
Two patches actually.
1. Timetracking Form Patch
2. Organization Id Patch.

The patch in #1 contains test for 1. I am not sure about the workflow, should we write test first or functions first.

If we go for test first, then it will necessarily mark all the pending patches as failed :(. ( This is why i wanted to roll the test case with the patch for forms ).

We need to find a way of hiding the permissions (such as 'create pmtimetracking') that core now adds for every new content type, or we use those instead of ours. Currently we have some duplicates that could confuse users.

Agreed, it is confusing.

The major issue is due to the fact that ADD permission supplied by Pm is not 'OR'ed with drupal's create permission. This is against normal drupal behaviour for permission which works on "whitelisting" principle.

I think we should implement hook_node_access for all the content types we are declaring Or maybe we can do away with Pm's ADD permission altogether.

( In hook_node_access, all we have to do would be return NODE_ACCESS_ALLOW for creating an OR condition )

The reason I doubt whether we should have it though, is because by having our own way of writing tests we raise the barrier for other Drupal developers wanting to quickly understand the code. How much time / coding does it save?

First i would answer this before getting into technicality.

While writing test first thing that i realized is we should have our own framework for testing. Why? cause to write any test case for pm would require a deep knowledge of how the entities are linked with each other. The above provided test is just a small subset of it, and you would immediately realize how much of those configuration can be reused to test lot of other functionalities.

Our pm setup is not simply enabling modules and testing it. It requires a considerable number of users and entities and the way they interact with these entities before a test is performed. And i believe it is going to grow more complex in the future.

In short,

A base setup which create N number of users and X number of entities to cover almost all the possibilities ( if not all ). This base setup would then be reused by our individual tests to set up the environment. ( thinking about DRY ).

This base setup can later on be reused or could eventually become a module in itself for demo content.

Otherwise we would find ourself copy pasting lots of codes from one test to another.

Its been just one day that i had started to work on simpletest. So need to study more about it and how conceptualize my idea in a drupalish way.

I have had a look at having a tests folder in a module and using hidden = TRUE we could create a module specifically for testing purpose.

All that would be required for a drupal test setup would be to include it as the last module to be enabled. Then we could just check which all modules are enabled and generate the base content.

Now i will answer other queries.

No need to set a nid, as core will do that for us, on an auto-increment.

I needed to store the node id of created node somewhere, since it will be used for referencing later on down the test. ( For Example while creating a timetracking entry does an Organization entry shows up for which he has permission, i can think of using title instead, since we can set title during node creation time and we can ensure its unique ).

I wonder whether it is better to have this function or not. Firstly, if we did have it, wouldnt' it be better in pm.test so that it would be the same for all pm modules?

I was thinking of putting it in a separate hidden module.

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented
FileSize
pm-pmtimetracking-test-1975514-05.patch9.97 KB

Made changes to the permission list as mentioned in #5.

Regarding my previous comment

I need more time to come up with most elegant solution for the demo content. I am pretty much sure that we could use demo content for testing as well as creating a demo content for user's to try and understand the PM and its permissions. I think the demo content could be a good feature for an Installation Profile for PM.

Status: Needs review » Needs work

The last submitted patch, pm-pmtimetracking-test-1975514-05.patch, failed testing.

juliangb’s picture

juliangb CreditAttribution: juliangb commented

Thanks for the updated patch.

To answer your question about which patch should be committed first, I suggest that we review them both, then once happy I can commit the functional changes. A double check that drupal.org thinks the tests pass, and we're on our way.

Good point about demo content and the tests - it would be ideal if these linked up and we had something that people could use to test Project Management. However, I'm not sure how this relates to the addition of the pmRunTest function - which seems to just allow us to specify the tests in an array rather than using the standard Drupal formatting.

My point on the nid is that we should be able to get this information from the node after it has been saved, rather than setting it ourself. If we started using the same code for demo content, we couldn't be sure that no nodes had been created already. That said, happy to keep that how it is for now, as it seems to work.

I need to look at this in more detail and try it out - and will comment again once I've done that.

juliangb’s picture

juliangb CreditAttribution: juliangb commented
Status: Needs work » Needs review

#7: pm-pmtimetracking-test-1975514-05.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, pm-pmtimetracking-test-1975514-05.patch, failed testing.

juliangb’s picture

juliangb CreditAttribution: juliangb commented

Tested this locally as well - and get the same failures.

It seems that the user can't access the organization add form to begin with.

Also, I'm still unconvinced by the need for the pmRunTest function. Could we not just use the standard commands?

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented

Sorry :( i think i will have to skip trying to create a test nodes through form simulating a user and just use

$this->drupalCreateNode($settings);

It returns a node and that way the test wont have to depend on keeping a node counter .

I will rewrite the whole test and submit it later. And yes it wont use the array either.

The test's scope would be reduced to test only for the patch supplied in Fix timetrackings form fields #8.

Rationale:
If we have other test written for checking form submission which would do an elaborate test against every field in that particular form, we don't need to recheck it in pmtimetracking and create for example organization node directly.

I also found a bug in test where it fails to detect the difference between a missing entry and an Access denied.

P.S. it will take a little while longer for me to figure out why its causing access denied. Maybe its related to the recent changes we had made in node permissions. I will confirm that later. Meanwhile if anybody can get the permission set up right, please share it here.

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented
FileSize
pm-pmtimetracking-test-1975514-09.patch7.6 KB
pm-pmtimetracking-test-1975514-09-bypass.patch7.72 KB

Update:

i have rewritten the whole test case, but it doesn't pass since node/add/pmtimetracking is inaccessible with some set of permission. I tried the test with bypass node access and was able to access the node add form. But organizations where not populated ( probably due to some condition added by 'node_access' tag in the query used to populate them).

Any pointers?

Also regarding the naming of function testpmtimetrackingCreate, should we add all the test to be done for creating in entry in this function ?

or make different test cases for testing aspects like creating an entry, access test, populating create form fields, edit test ... and the list goes on...

juliangb’s picture

juliangb CreditAttribution: juliangb commented

Thanks for pushing this.

I'm going to have a look at this, and have thought that actually we shouldn't necessarily block other features for this as we'll make sure it is in before we get to an official release.

I also wondered whether the organization tests were passing, and found that there is some work that needs doing further up the food chain first - am looking into it.

juliangb’s picture

juliangb CreditAttribution: juliangb commented

We need to make some changes to the access hooks for this to work, but it needs applying to all Project Management modules.

I've started the changes with #1981210: Port access controls and test, but we will need to work through the modules in order of dependencies as they all use the content types provided by each other. So I'll start working on the next once the org one is tested and committed.

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented

Once you provide a fix for Organizations, i could provide a patch for pmtimetracking, and if required for other entities too. But in case you are going to do a mass commit for all entities, do let me know. Don't want to duplicate the effort. Thank you.

dbt102’s picture

dbt102 CreditAttribution: dbt102 commented

+1

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, pm-pmtimetracking-test-1975514-09.patch, failed testing.

dbt102’s picture

dbt102 CreditAttribution: dbt102 commented
Status: Needs work » Needs review

Can't apply patch (neither of them).

D34dMan’s picture

D34dMan CreditAttribution: D34dMan commented

Yep i know it would fail. I wanted to make it clear that it needs rework with a proper message.

juliangb’s picture

juliangb CreditAttribution: juliangb commented
Status: Needs review » Needs work

Clarifying status.

juliangb’s picture

juliangb CreditAttribution: juliangb commented
Issue summary: View changes
Status: Needs work » Closed (won't fix)

We've not looked at this for 18months now, so I think it is time to close. We're now currently focussing on features rather than testing.