Comments

GoZ created an issue. See original summary.

GoZ’s picture

Status: Active » Needs review
FileSize
5.52 KB

I use -M and --find-copies-harder but it don't see GetNamespacesTest.php is moving and changed.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/rdf/tests/src/Functional/GetNamespacesTest.php
@@ -0,0 +1,36 @@
+    $this->assertTrue(strpos($prefix, 'rdfs: http://www.w3.org/2000/01/rdf-schema#') !== FALSE, 'A prefix declared once is displayed.');
+    $this->assertTrue(strpos($prefix, 'foaf: http://xmlns.com/foaf/0.1/') !== FALSE, 'The same prefix declared in several implementations of hook_rdf_namespaces() is valid as long as all the namespaces are the same.');
+    $this->assertTrue(strpos($prefix, 'foaf1: http://xmlns.com/foaf/0.1/') !== FALSE, 'Two prefixes can be assigned the same namespace.');
+    $this->assertTrue(strpos($prefix, 'dc: http://purl.org/dc/terms/') !== FALSE, 'When a prefix has conflicting namespaces, the first declared one is used.');

If you really want to use strpos you can use $this->assertContains instead which has some better readability.

mpdonadio’s picture

I use -M and --find-copies-harder but it don't see GetNamespacesTest.php is moving and changed.

The two files are pretty different, so I am not surprised they don't count as a move:

3c3
< namespace Drupal\rdf\Tests;
---
> namespace Drupal\Tests\rdf\Functional;
5c5
< use Drupal\simpletest\WebTestBase;
---
> use Drupal\Tests\BrowserTestBase;
13c13
< class GetNamespacesTest extends WebTestBase {
---
> class GetNamespacesTest extends BrowserTestBase {
29,47c29,33
<     $element = $this->xpath('//html[contains(@prefix, :prefix_binding)]', [
<       ':prefix_binding' => 'rdfs: http://www.w3.org/2000/01/rdf-schema#',
<     ]);
<     $this->assertTrue(!empty($element), 'A prefix declared once is displayed.');
< 
<     $element = $this->xpath('//html[contains(@prefix, :prefix_binding)]', [
<       ':prefix_binding' => 'foaf: http://xmlns.com/foaf/0.1/',
<     ]);
<     $this->assertTrue(!empty($element), 'The same prefix declared in several implementations of hook_rdf_namespaces() is valid as long as all the namespaces are the same.');
< 
<     $element = $this->xpath('//html[contains(@prefix, :prefix_binding)]', [
<       ':prefix_binding' => 'foaf1: http://xmlns.com/foaf/0.1/',
<     ]);
<     $this->assertTrue(!empty($element), 'Two prefixes can be assigned the same namespace.');
< 
<     $element = $this->xpath('//html[contains(@prefix, :prefix_binding)]', [
<       ':prefix_binding' => 'dc: http://purl.org/dc/terms/',
<     ]);
<     $this->assertTrue(!empty($element), 'When a prefix has conflicting namespaces, the first declared one is used.');
---
>     $prefix = $this->xpath('//.')[0]->getAttribute('prefix');
>     $this->assertTrue(strpos($prefix, 'rdfs: http://www.w3.org/2000/01/rdf-schema#') !== FALSE, 'A prefix declared once is displayed.');
>     $this->assertTrue(strpos($prefix, 'foaf: http://xmlns.com/foaf/0.1/') !== FALSE, 'The same prefix declared in several implementations of hook_rdf_namespaces() is valid as long as all the namespaces are the same.');
>     $this->assertTrue(strpos($prefix, 'foaf1: http://xmlns.com/foaf/0.1/') !== FALSE, 'Two prefixes can be assigned the same namespace.');
>     $this->assertTrue(strpos($prefix, 'dc: http://purl.org/dc/terms/') !== FALSE, 'When a prefix has conflicting namespaces, the first declared one is used.');

Attached is the straight move w/ rename+replace. It is going to fail, which concerns me. The same xpath expression that the test generates works in Chrome element inspect, which may mean we don't have a 1-to-1 conversion with xpath in WTB and BTB (and I don't see why this xpath is failing in BTB).

mpdonadio’s picture

Status: Needs work » Needs review

Go, testbot go.

Status: Needs review » Needs work

The last submitted patch, 4: 2864035-test-only.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
4.64 KB
1.87 KB

OK, I think the problem is that WTB::xpath() and BTB::xpath() work from different root elements (see http://mink.behat.org/en/latest/guides/traversing-pages.html).

This converts everything to use CSS traversal to get around the clunky xpath that you now have to use.

So, we may need a followup about BTB::xpath()...

Also noticed that Drupal\rdf\Tests\Field\TestDataConverter is orphaned, and not used in core (well, PHPStorm says so and it is smarter than me). Should we add a @deprecated to it in this patch? It's the only file left in core/modules/rdf/src/Tests now.

mpdonadio’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Also noticed that Drupal\rdf\Tests\Field\TestDataConverter is orphaned, and not used in core (well, PHPStorm says so and it is smarter than me). Should we add a @deprecated to it in this patch? It's the only file left in core/modules/rdf/src/Tests now.

I actually believe \Drupal\Tests\rdf\Kernel\Field\FieldRdfaDatatypeCallbackTest is using that ... you know callables :)

mpdonadio’s picture

That's what I get for trusting Find Usages and not searching for the string...

Should we move that file to live alongside the test that uses it? I suspect that was missed during #2684095: Convert field kernel tests to KernelTestBaseTNG. Or is that out of scope?

dawehner’s picture

Should we move that file to live alongside the test that uses it?

I think keeping the file where it is for now is totally cool. I actually like to not have tests and helper stuff in the same directory, but we could always deal with that in a followup, if you actually care.

mpdonadio’s picture

No string feelings one way or another. Let's commit this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ /dev/null
    similarity index 99%
    rename from core/modules/rdf/src/Tests/CommentAttributesTest.php
    
    rename from core/modules/rdf/src/Tests/CommentAttributesTest.php
    rename to core/modules/rdf/tests/src/Functional/CommentAttributesTest.php
    
    +++ b/core/modules/rdf/tests/src/Functional/CommentAttributesTest.php
    @@ -1,6 +1,6 @@
    -namespace Drupal\rdf\Tests;
    +namespace Drupal\Tests\rdf\Functional;
    

    This is still extending from a WebTestBase test... \Drupal\comment\Tests\CommentTestBase

  2. +++ b/core/modules/rdf/tests/src/Functional/GetNamespacesTest.php
    similarity index 98%
    rename from core/modules/rdf/src/Tests/ImageFieldAttributesTest.php
    
    rename from core/modules/rdf/src/Tests/ImageFieldAttributesTest.php
    rename to core/modules/rdf/tests/src/Functional/ImageFieldAttributesTest.php
    
    +++ b/core/modules/rdf/tests/src/Functional/ImageFieldAttributesTest.php
    @@ -1,6 +1,6 @@
    -namespace Drupal\rdf\Tests;
    +namespace Drupal\Tests\rdf\Functional;
    

    This doesn't look right. I think we also need to update \Drupal\image\Tests\ImageFieldTestBase to point at the browsertestbase version of the base test.

mpdonadio’s picture

Title: Convert web tests to browser tests for rdf module » [PP-1] Convert web tests to browser tests for rdf module
Status: Needs work » Postponed
Related issues: +#2866513: Convert web tests to browser tests for comment module
FileSize
5.01 KB
1.22 KB

#13-1 means we need to postpone on #2866513: Convert web tests to browser tests for comment module.

#13-2 is an easy switch, but the test fails b/c TestFileCreationTrait::drupalGetTestFiles() doesn't exist for BTB, so that test needs to be adjusted.

This patch will fail, but does the class switches from #13, but not the other fix.

mpdonadio’s picture

Title: [PP-1] Convert web tests to browser tests for rdf module » [PP-2] Convert web tests to browser tests for rdf module
Related issues: +#2863267: Convert web tests of views

And we are double-postponed, #2863267: Convert web tests of views

michielnugter’s picture

mpdonadio’s picture

No, I did mean #2863267: Convert web tests of views. That issue is blocking #2866513: Convert web tests to browser tests for comment module, which is blocking this one. I suspect we may also be blocked on the image module; I kinda gave up after digging through the comment-related tests and noticing we are at least double postponed (which kinda of makes sense considering what rdf.module does).

michielnugter’s picture

A right, I figured it wasn't directly postponed on views but via comment.

I've been working the issue queue and changing the scope for a lot of issues so they can be unpostponed but for the rdf module it doesn't make sense. Will probably take a while before we can RTBC this one..

dawehner’s picture

IMHO we should get the conversations in, and then make a final run for all the views related test classes. I strongly recommend for running things in parallel vs. serial :)

michielnugter’s picture

@dawehner: I agree completely but in this case there's not much left if we split all the tests that require another base class.

dawehner’s picture

Fair point :)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jonathan1055’s picture

jonathan1055’s picture

Title: [PP-2] Convert web tests to browser tests for rdf module » Convert web tests to browser tests for rdf module
Issue summary: View changes
Status: Postponed » Needs work
naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
4.3 KB

Here we go with the patch.

There's still one class left in the src/Tests namespace Drupal\rdf\Tests\Field\TestDataConverter which is used by the Drupal\Tests\rdf\Kernel\Field\FieldRdfaDatatypeCallbackTest This class conversion seems unrelated to this issue. Do we want to deal it here or in follow-up?

Status: Needs review » Needs work

The last submitted patch, 25: 2864035-25.patch, failed testing. View results

nlisgo’s picture

Assigned: Unassigned » nlisgo

I'm going to take a look at this issue and see if I can resolve some of these failing tests.

mpdonadio’s picture

I think this may be hard blocked on #2863626: Convert web tests to browser tests for image module now.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Needs work » Needs review
FileSize
368.56 KB
1.91 KB

@mpdonadio you may be right. I am addressing the failing tests in Drupal\Tests\rdf\Functional\GetNamespacesTest for now.

Targeted attributes of the html element are made a little bit more tricky by //html being prefixed onto each xpath query. There may be a better way to do this but these changes will perform the appropriate check.

Switching to Needs review to trigger testbot

Status: Needs review » Needs work

The last submitted patch, 29: convert_web_tests_to-2864035-29.patch, failed testing. View results

nlisgo’s picture

I have screwed up the patch in #29. I will resupply.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
5.65 KB
1.91 KB

I'm resupplying the patch intended for #29. I performed the diff for 8.4.x rather than 8.5.x.

Triggering testbot.

Status: Needs review » Needs work

The last submitted patch, 32: convert_web_tests_to-2864035-32.patch, failed testing. View results

nlisgo’s picture

Issue tags: +Vienna2017
nlisgo’s picture

Status: Needs work » Needs review
FileSize
5.65 KB
720 bytes

I introduced some whitespace into my last patch. Resupplying.

nlisgo’s picture

Issue summary: View changes
Status: Needs review » Postponed

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.