This is one of the tests that will need to be rewritten per #1866858: Test RDFa by parsing RDFa (add the easyrdf library).

dependencies that need to be committed first:
- #1848464: Make RDFa markup upward compatible with RDFa 1.1
- #1866858: Test RDFa by parsing RDFa (add the easyrdf library)

CommentFileSizeAuthor
#15 1867096-RdfaMarkupTest-15.patch7.47 KBscor
#13 1867096-RdfaMarkupTest-13__for-review.txt7.47 KBscor
#11 1867096-RdfaMarkupTest-7__for-review.txt7.44 KBlinclark
#7 1867096-RdfaMarkupTest-7__for-review.txt7.44 KBscor
#7 interdiff.txt9.28 KBscor
#7 1867096-RdfaMarkupTest-7__for-testing.patch436.44 KBscor
#3 1867096-RdfaMarkupTest-3__for-testing.patch427.41 KBscor
#3 1867096-RdfaMarkupTest-1__for-review.txt9.38 KBscor
#1 1867096-RdfaMarkupTest-1.patch427.41 KBscor
#1 1867096-RdfaMarkupTest-1__for-review.patch9.38 KBscor

Comments

scor’s picture

scor commented
Assigned: Unassigned » scor
Status: Active » Needs review
StatusFileSize
new 1867096-RdfaMarkupTest-1__for-review.patch9.38 KB
new 1867096-RdfaMarkupTest-1.patch427.41 KB

Here is a first patch which replaces all xpath expressions with proper RDF triple checks. It relies on #1866858: Test RDFa by parsing RDFa (add the easyrdf library) and #1848464: Make RDFa markup upward compatible with RDFa 1.1.

Edit: oops, messed up the extensions, the second one is for review and was not supposed to go for testing (should have been .txt).

Status: Needs review » Needs work

The last submitted patch, 1867096-RdfaMarkupTest-1__for-review.patch, failed testing.

scor’s picture

scor commented
Status: Needs work » Needs review
StatusFileSize
new 1867096-RdfaMarkupTest-1__for-review.txt9.38 KB
new 1867096-RdfaMarkupTest-3__for-testing.patch427.41 KB

can't reproduce the locale.module test fail on localhost, so I'm assuming it's a random testbot failure. re-uploading patches with better filenames (no code changes).

Status: Needs review » Needs work

The last submitted patch, 1867096-RdfaMarkupTest-3__for-testing.patch, failed testing.

scor’s picture

scor commented
Status: Needs work » Needs review

testbot is happy now... back to NR

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work 
+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/RDFTestHelper.phpundefined
@@ -0,0 +1,31 @@
+  public static function GraphContainsTriple($graph, $triple) {

Method name should start with a lowercase letter.

Other than that, I think that this is a huge improvement. If you can make that small change, I'll RTBC today :)

scor’s picture

scor commented
Status: Needs work » Needs review
StatusFileSize
new 1867096-RdfaMarkupTest-7__for-testing.patch436.44 KB
new interdiff.txt9.28 KB
new 1867096-RdfaMarkupTest-7__for-review.txt7.44 KB

Good catch, Lin. Luckily we no longer need that method since @njh was kind enough to add this feature in easyrdf for us. Here is the updated patch to use the new feature of the hasProperty() method in our tests.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

Once #1866858: Test RDFa by parsing RDFa (add the easyrdf library) and #1848464: Make RDFa markup upward compatible with RDFa 1.1 are in, 1867096-RdfaMarkupTest-7__for-review.txt can be committed.

This is a huge improvement over the XPath based tests because it actually checks how the RDFa is interpreted by a parser.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Postponed

Marking this postponed those issues for now. I got very confused why two issues were trying to add the same 3rd party library. :D

scor’s picture

scor commented

well, this issue depends on that other issue (#1866858: Test RDFa by parsing RDFa (add the easyrdf library)) to have the library added like it says in the OP. the patch filenames should be clear: the '__for_testing' includes the library (packaged here for convenience for the testbot), and the patch to review is the '__for_review' and it does not include the library, it's that patch that should be committed once the library has been added (from the other issue).

Note that there will be more patches like this one relying on this library (wherever we test RDFa output using XPath).

Anonymous’s picture

Anonymous (not verified) commented
Status: Postponed » Reviewed & tested by the community
StatusFileSize
new 1867096-RdfaMarkupTest-7__for-review.txt7.44 KB

As I explained to Moshe, test issues against RDF will hold up JSON-LD. We're at the point in JSON-LD development where we need to start bringing in the RDF parts of it, but we can't do major overhauls of the RDF module if we aren't sure that our tests are checking the right thing.

I'm attaching the patch which should be committed and moving back to RTBC. Note that it depends on the issues listed in the issue summary (which are also listed in my comment in #8).

Anonymous’s picture

Anonymous (not verified) commented
Status: Reviewed & tested by the community » Postponed

Ah, nevermind, I see webchick just postponed on those issues, not indefinitely. I'll move back to postponed, but I hope it can be committed today.

scor’s picture

scor commented
StatusFileSize
new 1867096-RdfaMarkupTest-13__for-review.txt7.47 KB

this patch needed a reroll after the XPath expressions changes from #1848464: Make RDFa markup upward compatible with RDFa 1.1. These XPath are removed by this patch, but they need to match nonetheless otherwise git/patch will complain when applying the patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Postponed » Needs review

#1866858: Test RDFa by parsing RDFa (add the easyrdf library) Just got committed, unpostponing this one. :)

scor’s picture

scor commented
StatusFileSize
new 1867096-RdfaMarkupTest-15.patch7.47 KB

rerolling so we can get the green

scor’s picture

scor commented
Status: Needs review » Reviewed & tested by the community

green = back to RTBC

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks! Sorry that fell off my radar. :(

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

add dependencies