Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#23 | 1869914-22-RdfMappingDefinitionTestCase.patch | 16.4 KB | scor |
#21 | 1869914-21-RdfMappingDefinitionTestCase.patch | 17 KB | scor |
#21 | interdiff.txt | 9.33 KB | scor |
#20 | 1869914-20-RdfMappingDefinitionTestCase.patch | 36.65 KB | scor |
#20 | interdiff.txt | 9.33 KB | scor |
Comments
Comment #1
scor CreditAttribution: scor commentedComment #2
scor CreditAttribution: scor commentedthis 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.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for posting this, just starting to review it now.
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.
Comment #4
scor CreditAttribution: scor commentedthe standard install profile isn't useful for TaxonomyAttributesTest and UserAttributesTest, but it is used as text fixture for NodeAttributesTest and CommentAttributesTest.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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.
Comment #7
scor CreditAttribution: scor commentedI 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.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedAs 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.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedForgot to mark NW
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedActually, 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.
Comment #12
scor CreditAttribution: scor commentedYou're right, I've reduced NodeAttributesTest to one test, and re-created MappingDefinitionTest to only test the RDF mapping definitions.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedIt seems this patch no longer applies. Also, I'm thinking that the NodeAttributesTest can probably use EasyRDF now.
Comment #14
scor CreditAttribution: scor commentedyes, 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!
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedIt seems like the rename of the comment and tracker tests could be in the respective patches for those, which already have issues.
In multiple places, this should be "Contains" rather than "Definition of".
Other than that, this looks pretty ready to me.
Comment #16
scor CreditAttribution: scor commentedfixed all that plus a test assertion string.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedShoot, patch doesn't apply again...
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedWhoops, me being silly.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedJust reviewed the NodeAttributes, but I think the comment will probably apply to all three new tests.
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.
This variable isn't used.
Comment #20
scor CreditAttribution: scor commentedaddressed #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.
Comment #21
scor CreditAttribution: scor commentedoops, forgot to rebase, 8.x keeps moving ahead...
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for separating the code more cleanly, this is looking good to me.
Comment #23
scor CreditAttribution: scor commentedthis needs a reroll actually...
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedSince the reroll is green, I'm moving it back to RTBC.
Comment #25
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.