Problem/Motivation

As was pointed out on IRC by linclark, RdfMappingDefinitionTestCase include tests which effectively go beyond testing the actual Mapping definitions, since they go as far as testing the markup. The test case name should reflect that.

Proposed resolution

Split the test methods of RdfMappingDefinitionTestCase into their own test cases which will test the markup like it is already the case with CommentAttributesTest and TrackerAttributesTest.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scor’s picture

Title: Refactor RdfMappingDefinitionTestCase into » Refactor RdfMappingDefinitionTestCase and split it in dedicated test cases
scor’s picture

Status: Active » Needs review
FileSize
18.46 KB

this patch splits MappingDefinitionTest into NodeAttributesTest, TaxonomyAttributesTest and UserAttributesTest. It also updates the label of a couple of other test cases to match the same pattern as the new test cases.

Anonymous’s picture

Status: Needs review » Needs work

Thanks for posting this, just starting to review it now.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/NodeAttributesTest.phpundefined
@@ -0,0 +1,97 @@
+
+  /**
+   * Use the standard profile.
+   *
+   * @var string
+   */
+  protected $profile = 'standard';

I'm not sure why we use a profile instead of just enabling modules. Wouldn't enabling node and rdf be sufficient? Using the full profile makes the test runs slower.

-17 days to next Drupal core point release.

scor’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
18.24 KB

the standard install profile isn't useful for TaxonomyAttributesTest and UserAttributesTest, but it is used as text fixture for NodeAttributesTest and CommentAttributesTest.

Anonymous’s picture

I'm not sure what you mean by text fixture. I tested the NodeAttributesTest without the standard profile but adding node to the modules and it gave me the same result. Granted, the test had fails because I tested it in the branch I'm working in, but the fails were exactly the same.

Status: Needs review » Needs work

The last submitted patch, 1869914_4_RdfMappingDefinitionTestCase_split.patch, failed testing.

scor’s picture

Status: Needs work » Needs review
FileSize
3.81 KB
17.16 KB

I got 3 errors on NodeAttributesTest.php:47 when removing the standard profile (node is a already dependency of the testing profile).

I've removed the standard profile from NodeAttributesTest since we didn't really test anything specific to the install profile there anyways (this is done in another test). I've adapted the xpath expression to the stark theme (nodes are wrapped in an article element in stark, and in a div in bartik). I've also removed the first test method since it doesn't really make sense anymore to create a node of type article in the testing install profile).

CommentAttributesTest still requires the standard install profile for a lot of the settings of comments, and generates a lot of fails if it's not there.

Anonymous’s picture

As we talked about in IRC, it's probably best to switch to using the 'entity_test' entity/bundle type.

Additionally, NodeAttributesTest::testAttributesCustomMappings() also ends up testing that the default mapping for 'node' is used when it tests for dc:title and other properties. Is this what was intended?

Additionally, I don't think we need to have two tests in this file. Whether a mapping can be saved via the .install (or whether a default mapping is loaded) is orthogonal to how it is displayed in the HTML. I would expect this test to simply set up one mapping in the .install file which had all of the required properties and then test that all had been placed appropriately.

So to summarize, we should test that mappings can be saved in all the expected ways. We should separately test that a mapping can be placed in HTML properly.

Anonymous’s picture

Forgot to mark NW

Anonymous’s picture

Status: Needs review » Needs work
Anonymous’s picture

Actually, it was silly of me to suggest that we use the entity_test entity, since all of these are testing particular entity types. Disregard the first sentence of that comment.

scor’s picture

Status: Needs work » Needs review
FileSize
6.59 KB
19.05 KB

You're right, I've reduced NodeAttributesTest to one test, and re-created MappingDefinitionTest to only test the RDF mapping definitions.

Anonymous’s picture

Status: Needs review » Needs work

It seems this patch no longer applies. Also, I'm thinking that the NodeAttributesTest can probably use EasyRDF now.

scor’s picture

Status: Needs work » Needs review
FileSize
11.42 KB
22.53 KB

yes, totally, that's why I had added this issue in the list in #1866858: Test RDFa by parsing RDFa (add the easyrdf library) :)

switched to using easyrdf for testing RDFa in the new files this issue creates. such an improvement!

Anonymous’s picture

Status: Needs review » Needs work

It seems like the rename of the comment and tracker tests could be in the respective patches for those, which already have issues.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/NodeAttributesTest.phpundefined
@@ -0,0 +1,85 @@
+ * Definition of Drupal\rdf\Tests\NodeAttributesTest.

In multiple places, this should be "Contains" rather than "Definition of".

Other than that, this looks pretty ready to me.

scor’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
21.38 KB

fixed all that plus a test assertion string.

Anonymous’s picture

Status: Needs review » Needs work

Shoot, patch doesn't apply again...

Anonymous’s picture

Status: Needs work » Needs review

Whoops, me being silly.

Anonymous’s picture

Status: Needs review » Needs work

Just reviewed the NodeAttributes, but I think the comment will probably apply to all three new tests.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/NodeAttributesTest.phpundefined
@@ -0,0 +1,85 @@
+   * Tests if RDF mapping defined in rdf_test.install is found in the markup.

Why are we testing if the rdf_test.install mapping is found in the HTML? The important thing is that we can place all of the mapping's attributes, not how they made it in to the mapping. We should just test the attributes from the default bundle, since that provides coverage for all of the RDFa attributes that core places on the node, IIRC.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/NodeAttributesTest.phpundefined
@@ -0,0 +1,85 @@
+    $type = $this->drupalCreateContentType(array('type' => 'test_bundle_hook_install'));

This variable isn't used.

scor’s picture

Status: Needs work » Needs review
FileSize
9.33 KB
36.65 KB

addressed #19 by refactoring the start of NodeAttributesTest. removed UserAttributesTest which is now in #1895064: Ensure user name is language neutral in RDFa and update tests to parse RDFa.

scor’s picture

oops, forgot to rebase, 8.x keeps moving ahead...

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for separating the code more cleanly, this is looking good to me.

scor’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
16.4 KB

this needs a reroll actually...

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Since the reroll is green, I'm moving it back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Status: Fixed » Closed (fixed)

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