Support from Acquia helps fund testing for Drupal Acquia logo

Comments

edurenye created an issue. See original summary.

edurenye’s picture

Status: Active » Needs review
FileSize
522 bytes

Added the views schema.

Berdir’s picture

Probably needs tests, we need a test view that adds a relationship so we can test the schema.

socketwench’s picture

Could we leverage Flag Bookmark for the test view? It seems like a natural place.

edurenye’s picture

Added the test and solved some errors that I found doing the test.

The last submitted patch, 6: add_views_schema_for-2555365-6-test_only.patch, failed testing.

joachim’s picture

  1. +++ b/flag_bookmark/config/install/flag.flag.bookmark.yml
    @@ -22,9 +22,9 @@ flagTypeConfig:
    diff --git a/flag_bookmark/src/Tests/FlagBookmarkUITest.php b/flag_bookmark/src/Tests/FlagBookmarkUITest.php
    
    diff --git a/flag_bookmark/src/Tests/FlagBookmarkUITest.php b/flag_bookmark/src/Tests/FlagBookmarkUITest.php
    new file mode 100644
    
    new file mode 100644
    index 0000000..f49af65
    
    index 0000000..f49af65
    --- /dev/null
    
    --- /dev/null
    +++ b/flag_bookmark/src/Tests/FlagBookmarkUITest.php
    
    +++ b/flag_bookmark/src/Tests/FlagBookmarkUITest.php
    +++ b/flag_bookmark/src/Tests/FlagBookmarkUITest.php
    @@ -0,0 +1,77 @@
    
    @@ -0,0 +1,77 @@
    +<?php
    +/**
    + * @file
    + * Contains \Drupal\flag_bookmark\Tests\FlagBookmarkUITest.
    + */
    

    > Could we leverage Flag Bookmark for the test view? It seems like a natural place.

    I'm not sure that's really giving us anything -- the test has to depend on the submodule. Do we even need a predefined view for this anyway?

  2. +++ b/flag_bookmark/src/Tests/FlagBookmarkUITest.php
    @@ -0,0 +1,77 @@
    +    // Add articles.
    +    $this->drupalPostForm('node/add/article', [
    +      'title[0][value]' => 'Article 1',
    +    ], t('Save'));
    

    Can we make this node programatically instead? We're not testing node module's UI.

  3. +++ b/flag_bookmark/src/Tests/FlagBookmarkUITest.php
    @@ -0,0 +1,77 @@
    +    // Check the front page does not have bookmark link.
    

    I'm confused about this bit!

socketwench’s picture

Status: Needs review » Needs work

@joachim I think you meant to mark this as needs work?

Berdir’s picture

Status: Needs work » Needs review

3. you're confused because the test has nothing to do with bookmark the way it is. It alters the frontpage view. I just showed that to @edureyne as an example for manual testing, *not* to do that in an automated test :) That doesn't make sense.

I think that writing a test for flag_bookmark *does* make sense. Because it already has a view with exactly such a relationship, so just having a test that enabled flag_bookmark should already be enough to trigger the schema error? And it does, you can see that nicely in the failing test, it doesn't fail on those frontpage view changes, it already fails on importing the bookmark view.

Berdir’s picture

Status: Needs review » Needs work

Crosspost.

joachim’s picture

> I think that writing a test for flag_bookmark *does* make sense. Because it already has a view with exactly such a relationship,

True.
But it means that if we ever change the bookmark view for some reason, it affects our test.
Also, if a site builder alters that view, and uses core and contrib tests as part of their CI, then that could break their tests too.

I'm maybe worrying too much ;)

Berdir’s picture

But it means that if we ever change the bookmark view for some reason, it affects our test.

Yes. But you can also add a safe guard by checking if the view has a relationship. But that *is* all the bookmark view is about. that relationship :) And we should have such a test anyway.

Also, if a site builder alters that view, and uses core and contrib tests as part of their CI, then that could break their tests too.

No. Tests always use default config.

edurenye’s picture

I will continue with this when this issue: https://www.drupal.org/node/2562971 is fixed. As it's not possible to activate this view now.

edurenye’s picture

Status: Needs work » Postponed
socketwench’s picture

Status: Postponed » Active

#2562971: Flag bookmark view is broken Is fixed, setting to active.

Berdir’s picture

Status: Active » Needs review
FileSize
7.12 KB

Reuploading the patch from the referenced issue.

+++ b/flag_bookmark/config/install/views.view.flag_bookmark.yml
@@ -458,14 +410,12 @@ display:
         description: ''
-        name: main
         weight: 0

not sure if this is correct.

socketwench’s picture

If it isn't, I don't immediately see a problem.

  • Berdir authored a7ab094 on 8.x-4.x
    Issue #2555365 by edurenye, Berdir: Added views schema for...
socketwench’s picture

Status: Needs review » Fixed

Since this was previously reviewed in another issue, and the tests apparently pass, let's commit it.

Thanks!

Status: Fixed » Closed (fixed)

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