In the OverviewPageTest there's a todo: "Why is this here? This hasn't got anything to do with the overview."

I removed the code and updated the test method to be somewhat more specific.

CommentFileSizeAuthor
#8 2642998-8--remove_unused_overview_page_test_code.patch4.08 KBdrunken monkey
#4 2642998-4--remove_unused_overview_page_test_code.patch4.13 KBdrunken monkey
#4 2642998-4--remove_unused_overview_page_test_code--interdiff.txt1.84 KBdrunken monkey
#3 remove_unused_code_from-2642998-3.patch3.93 KBborisson_
#3 interdiff.txt929 bytesborisson_
update-overview-page-test.patch3.02 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisson_ created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Needs review » Needs work

Looks good, thanks!
However, why the changes in the WebTestBase?
Also, I guess we should still test this somewhere, so maybe move the code to the WebTestBase in some way instead of just removing it? (It probably creates a lot of changes if we add the User datasource to our normal test index, so just creating another index (or adding it to the second one that, I believe, is already there somewhere) is probably the safer option.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented
Status: Needs work » Needs review
FileSize
interdiff.txt929 bytes
remove_unused_code_from-2642998-3.patch3.93 KB

The changes in WebTestBase are unrelated but they are to make sure that we have sufficient difference between the title and description of a index / server when testing.
Because if they are the same, then the following asserts are risky, because they don't actually test for something different.

    $this->assertText($index->label(), 'Index present on overview page.');
    $this->assertRaw($index->get('description'), 'Index description is present');

I have moved the test to the normal integrationTest.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
FileSize
2642998-4--remove_unused_overview_page_test_code--interdiff.txt1.84 KB
2642998-4--remove_unused_overview_page_test_code.patch4.13 KB

As noted elsewhere, I really don't like adding additional test methods to Simpletest tests, it's just too expensive. We can just do it in a separate helper method for the existing test (we currently already create a second, unused index in createIndex()).

$this->assertRaw($index->get('description'), 'Index description is present');

This actually looks like we should have HTML in the description, to check this realistically? (Not sure if that really works at the moment. And if it does, we should make sure there's no XSS problem there (i.e., some very permissive tag filter is applied).)
However, I guess that is a different issue.

Thanks for your work on the patch, in any case!

Status: Needs review » Needs work

The last submitted patch, 4: 2642998-4--remove_unused_overview_page_test_code.patch, failed testing.

The last submitted patch, 4: 2642998-4--remove_unused_overview_page_test_code.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented

The change you made from assertRaw to assertText made the tests red. Your other changes look good.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Needs work » Needs review
FileSize
2642998-8--remove_unused_overview_page_test_code.patch4.08 KB

The change you made from assertRaw to assertText made the tests red. Your other changes look good.

I didn't change that (or anything, functionally), the patch just needed a re-roll.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented
Status: Needs review » Reviewed & tested by the community

oops, yes.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Reviewed & tested by the community » Fixed

OK, then: committed.
Thanks again!

Status: Fixed » Closed (fixed)

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