File field display tests not working correctly [#2603710]

Problem

While exploring unit test cases codebase for File module, I think I found two minor bugs in testNodeDiplay() in FileFieldDisplayTestCase class:
1) Test for checking that the file field appears as expected on node view pages uses "/node/$nid/edit" instead of "/node/$nid/" in assertRaw() method.
2) Test for checking that the file field is no longer displayed when the "display" option is turned off passes even when the "display" option is turned on.

Steps to reproduce bug

1) Turn the "display" option on for file field by replacing FALSE with TRUE at line 937 in file.test.
2) Run "File Field Display tests" for File module.
3) Manually see from the appropriate debug message that the file field was still not shown and therefore the test passed successfully.

Proposed resolution

1) For problem#1 mentioned above, change drupalGet('node/' . $nid . '/edit') with drupalGet('node/' . $nid) for the perspective test case.
2) For problem#2 mentioned above, manually debug to see where is the problem.

Remaining tasks

1) A patch is ready for review which fixes both bugs.
2) Root cause for problem#2 mentioned above is still unknown.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Comment	File	Size	Author
#7	file_field_display-2603710-7.patch	2.16 KB	talhaparacha
#7	7.x: PHP 5.3 & MySQL 5.5 1,999 pass
#2	file_field_display-2603710-2.patch	5.03 KB	talhaparacha
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

28 October 2015 at 19:29

talhaparacha created an issue. See original summary.

Comment #2

talhaparacha CreditAttribution: talhaparacha as a volunteer commented 28 October 2015 at 19:37

Status:

Active

» Needs review

File	Size
file_field_display-2603710-2.patch	5.03 KB

Problem#1 has been fixed as per the proposed solution. To fix Problem#2, test cases in testNodeDisplay() have been expanded into two methods; testDiplayOnNodeWithoutFile() and testDisplayOnNode(). That's because I think drupalCreateNode() (for creating a new node without file field set) and uploadNodeFile() (for creating a new node with file) calls in the same method testNodeDisplay() were conflicting with each other and needed to be separated. I've also tried to refactor some code as per the Drupal coding standards.

Comment #3

cilefen CreditAttribution: cilefen commented 28 October 2015 at 20:53

Title:	File field display tests not working correctly!	» File field display tests not working correctly
Issue tags:	-Testing system

Please check if this is an issue with Drupal 8. If so, it must be fixed there first according to the backport policy. If it is, move it to branch 8.0.x-dev and tag it “Needs backport to D7”. There may also be an existing Drupal 8 issue. Try to find it. If one exists, tag it “Needs backport to D7”. If it is open, offer a patch. If its status is “Fixed”, make its status “Patch (to be ported)”, move it to version 7.x-dev and upload your latest patch if one exists.

Comment #4

talhaparacha CreditAttribution: talhaparacha as a volunteer commented 2 November 2015 at 16:11

Thanks for the information about backport policy. I checked and the issue does not apply for Drupal 8, both test assertions there are working fine and as expected. Now waiting for the review of patch @ 2 for Drupal 7.

Comment #5

talhaparacha CreditAttribution: talhaparacha as a volunteer commented 21 November 2015 at 09:59

Assigned:

talhaparacha

» Unassigned

Comment #6

penyaskito

he/him

Spanish

🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺

CreditAttribution: penyaskito as a volunteer commented 22 March 2016 at 23:45

Status:

Needs review

» Needs work

+++ b/modules/file/tests/file.test
@@ -904,40 +904,70 @@ function testNodeDisplay() {
-    $this->createFileField($field_name, $type_name, $field_settings, $instance_settings, $widget_settings);
+    $this->createFileField($field_name, $type_name, $field_settings,
+      $instance_settings, $widget_settings);
...
-    $file_formatters = array('file_default', 'file_table', 'file_url_plain', 'hidden');
+    $file_formatters = array('file_default', 'file_table', 'file_url_plain',
+      'hidden');
...
-      $this->drupalPost("admin/structure/types/manage/$type_name/display", $edit, t('Save'));
+      $this->drupalPost("admin/structure/types/manage/$type_name/display",
+        $edit, t('Save'));

We shouldn't be changing the formatting of this lines, it's hard to be focused on review the relevant changes. Maybe an IDE automatic change?

+++ b/modules/file/tests/file.test
@@ -904,40 +904,70 @@ function testNodeDisplay() {
-    $this->assertRaw($default_output, 'Default formatter displaying correctly on full node view.');
+    $this->assertRaw($default_output, 'Default formatter displaying correctly on
+      full node view.');
...
-    // Turn the "display" option off and check that the file is no longer displayed.
+    // Turn "display" option off and check that the file is no longer displayed.

Same here.

Otherwise looks good to me.
Thanks for your contribution!