Although #1551248: Port project-specific "Assigned" functionality to D7 is now fixed, I realized that we somewhere lost the tests for the issue maintainer functionality that we used to have in D6. It'd be nice to port all these to D7.

Level Of Effort

It is estimated to take less than one day to complete this task.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Senpai’s picture

Issue tags: +low hanging fruit
paulovelho’s picture

FileSize
3.8 KB

The test was having some troubles to find the options in the select box.

I had to change the options sent to the options that were being displayed in the web page. The name of the select box was different from the expected as well.

I'm not quite sure how the options and names are being created, but at least the tests are successful now.

There is still some notices in the test result. Like this one:

Undefined property: ProjectIssue_SelectionHandler_Assigned::$entity Notice ProjectIssue_SelectionHandler_Assigned.class.php 59 ProjectIssue_SelectionHandler_Assigned->getAllowedAssignees()

paulovelho’s picture

Status: Active » Needs review
Nick_vh’s picture

Thanks Paulo! A patch in your first two hours in the Drupal world! Well done!
Hurray for the Drupal.org upgrade sprint. Go London team ;-)

+++ b/tests/integration/maintainers.testundefined
@@ -52,12 +53,12 @@ class ProjectIssueMaintainersTestCase extends ProjectIssueWebTestCase {
+    $this->assertSelectFieldOptions('field_issue_assigned[und]', array(t('- None -'), t('Anonymous'), $this->maintainer->name, $this->owner->name, t('placeholder-for-uid-1')));

Can someone confirm if this can be improved so we do not have to define the [und]?

Can someone tell us what this placeholder is?

bdragon’s picture

The placeholder is the language. LANGUAGE_NONE is 'und', you will see it all the time when working with fields.

Nick_vh’s picture

I was referring to "placeholder-for-uid-1". Can you clarify that?

dww’s picture

Status: Needs review » Needs work

Cool, thanks for the patch and work on this! However, there are still some lingering problems here:

A) Just visiting the simpletest page, I'm seeing this warning:

Strict warning: Declaration of ProjectMaintainersTestCase::setUp() should be compatible with that of ProjectWebTestCase::setUp() in _registry_check_code() (line 3096 of /.../includes/bootstrap.inc).

B) when I run the tests, I'm getting a 16 failures, most of them like this:

The option Anonymous was expected, but not found.
The option placeholder-for-uid-1 was expected, but not found.

I agree the 'placeholder-for-uid-1' stuff is bogus and should be removed. Similarly, 'Anonymous' shouldn't be there at all (as explained at #1551248: Port project-specific "Assigned" functionality to D7).

C) However, there are 4 failures like this:

The option x4h1Jay1 was expected, but not found.

Those are on lines 56, 61, 92, and 96 of the patched maintainers.test file. So, each of those 4 assertions needs to be investigated to see if it's a bogus test or an actual bug. ;)

Thanks,
-Derek

rgristroph’s picture

Assigned: Unassigned » rgristroph

I'm taking a look at this, assigning to myself.

--Rob

dww’s picture

Assigned: rgristroph » Unassigned
Issue tags: +3hr

Rough estimate of 3 hours. I don't think rgristroph is actively working on this. Please re-assign to yourself if I'm wrong. ;)

Thanks,
-Derek

drumm’s picture

Priority: Normal » Minor
Issue tags: -3hr
drumm’s picture

Issue tags: -low hanging fruit, -drupal.org D7 +Drupal.org 7.1
drumm’s picture

Issue summary: View changes

Adding a level of effort.

kalman.hosszu’s picture

Assigned: Unassigned » kalman.hosszu
Issue summary: View changes

I'm starting to check this one.

kalman.hosszu’s picture

Status: Needs work » Needs review
FileSize
5.34 KB

I attached a patch. It works with my localhost but please review it.
Entityreference module generates a string for "Assigned" select element options called "- Restricted access -" so I checked the string too, if you have any better idea to check that test case, please write it to me.

drumm’s picture

Status: Needs review » Fixed

Thanks!

drumm’s picture

Status: Fixed » Needs work

https://qa.drupal.org/pifr/test/268788 says:

  • The option umTHB7x9 was found, but not expected.
  • The option - Restricted access - was expected, but not found.
kalman.hosszu’s picture

Status: Needs work » Needs review

It's interesting, I just ran the project tests in my localhost and it was passed "174 passes, 0 fails, 0 exceptions, and 54 debug messages"

Could you re-run the tests?

drumm’s picture

I re-ran the tests, https://qa.drupal.org/pifr/test/268788. I think it has the same failed results as I mentioned in #16.

The most details are under "Review log". It could be a tagged version of a dependency was used instead of -dev. There is also an FAQ in the sidebar.

I'd be interested to know if the tests pass on a Drupal.org dev site.

marvil07’s picture

Assigned: kalman.hosszu » Unassigned
FileSize
1.3 KB

The included patch depends on #2262415: Test's setUp() method should be protected to avoid a php fatal(a tests inherits from a project test class which contains the same visibility bug).
It fixes the strict warning mentioned on comment 7.

The usual problem with both is one of the mentioned items in the FAQ. Sadly, even running tests from CLI, with the method testbot uses I could not yet reproduce the problem with my local install(even from a from-scratch site).
FTR the line I'm using:

php ./scripts/run-tests.sh --verbose --url http://here.my.local.vhost --file sites/all/modules/project_issue/tests/integration/issue_creation.test,sites/all/modules/project_issue/tests/integration/maintainers.test,sites/all/modules/project_issue/tests/project_issue.test --list 2>&1

I have decided to review the dependencies on the testbot run as drumm suggested, which ended up in some strange findings:
A. entityreference-7.x-1.0 is used instead of the current stable 7.x-1.1 (not recent, so sounds like fixed version dependency somewhere). Two assertions from ProjectIssueMaintainersTestCase fails downgrading this to testbot used version (same as testbot).
B. project version used is 7.x-2.0-unstable1, which is really old (2013-Jan-22), new release please :-). The other failing assertion, from ProjectIssueMaintainersTestCase can be reproduced downgrading to that version.

In both cases I did not look at the details yet, but it seems like it's better to just update the dependency.

For A. I do not have a solution, I could not find any info file using an explicit version dependency for entityreference. Maybe a dependency builder bug? Not sure.
For B a new release for project project is needed.

  • Commit 0dd919d on 7.x-2.x by drumm:
    #1633020 Explicitly depend on project -dev.
    
drumm’s picture

The patch in #19 looks the same as #2262415: Test's setUp() method should be protected.

So far, #20 hasn't made a difference. I think it may take awhile to pick up on it.

marvil07’s picture

A and B mentioned on comment 19 are still happening(I have just reviewed the log again on https://qa.drupal.org/pifr/test/268788), so yes, we need to wait the d.o project dependencies rebuild.

Sorry about the patch, it seems I confused it while uploading. Added now a new one, please notice this will not fix the real problems but use the right visibility for the setUp() test methods.

  • Commit 2497f17 on 7.x-2.x authored by marvil07, committed by drumm:
    Issue #1633020 follow-up: Test's setUp() method should be protected.
    

  • Commit 61bc6cc on 7.x-2.x by drumm:
    #1633020 Explicitly depend on project -dev for tests.
    

  • Commit 97e9e08 on 7.x-2.x by drumm:
    #1633020 Explicitly depend on entityreference 7.x-1.1 for tests.
    

Status: Needs review » Needs work
drumm’s picture

Status: Needs work » Fixed

Thanks everyone! The branch tests now pass. #26 is of course the testbot seeing if it can apply the patch a second time.

#24 was the trick for getting -dev instead of unstable1 for project. The testbot rightfully wants to test against more-stable dependencies.

I don't know if #25 is actually required, but it doesn't hurt to be explicit. The actual fix was to run the project_dependency_rebuild_project Jenkins job on entityreference. For whatever reason, the project dependency data didn't know about 1.1 at all.

Status: Fixed » Closed (fixed)

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