As noticed in #1831632: Convert node_rank_ $rank variables to use configuration system there should be a test for the UI that controls the node_rank_$rank variables (or their future CMI replacements). The existing tests for search ranking set the variables/config directly instead of using the UI, so the UI is never directly tested.

CommentFileSizeAuthor
#13 2132647-11-13-interdiff.txt1.48 KBianthomas_uk
#13 2132647-nodesearch-settings-test-13.patch3.3 KBianthomas_uk
#11 2132647-nodesearch-settings-test-11.patch3.28 KBianthomas_uk
#11 2132647-9-11-interdiff.txt2.34 KBianthomas_uk
#9 human-git-interdiff.txt1.13 KBianthomas_uk
#9 2132647-nodesearch-settings-test-9-for-humans-do-not-test.patch3.43 KBianthomas_uk
#9 2132647-nodesearch-settings-test-9.patch3.37 KBianthomas_uk
#3 2132647-nodesearch-settings-test-3.patch5.95 KBianthomas_uk
#1 2132647-nodesearch-settings-test-1.patch2.21 KBianthomas_uk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Active » Needs review
FileSize
2132647-nodesearch-settings-test-1.patch2.21 KB

Here's a test

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

All this tests is that if you change the settings on the settings form, the next time you go to the form the numbers are updated.

It needs to test whether this actually affects searching in the desired way.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Needs work » Needs review
FileSize
2132647-nodesearch-settings-test-3.patch5.95 KB

I suggested to jhodgson on IRC that the node creation should be moved to setUp(), but have since changed my mind as the various tests using different very different nodes and it just made the tests harder to read.

The attached patch therefore just converts the first test to use the UI rather than modifing the variables directly. I've also commented out some unnecessary setup that wasn't being used in the test (designed for views/comments testing).

No interdiff as it's a new approach.

Status: Needs review » Needs work

The last submitted patch, 3: 2132647-nodesearch-settings-test-3.patch, failed testing.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

3: 2132647-nodesearch-settings-test-3.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

We don't normally like to see commented-out code, especially not such large stretches of it. I think that the code that adds the comments to the node and views the node can be left there -- what harm would it do to have it there? This test presumably used to pass without commenting out all that stuff -- let's just leave it as much like it was as possible, and just make the changes that will use the UI rather than variable_set(). OK?

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

It's just unnecessary work for the test script, as the nodes created aren't used in the test.

The test worked before because views and comments were in the array before the commented out lines, and then removed before running the tests.

I'll go back to the old way of doing it though if that's the standard.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Personally, I think that one line with a @todo saying "Remove this line when views and comments ranking are working" is a lot easier to deal with than 30 lines of commented-out code. Plus, this patch is a lot easier to review if the only thing it changes is to use the UI rather than the variable_set or config or whatever.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
FileSize
2132647-nodesearch-settings-test-9.patch3.37 KB
2132647-nodesearch-settings-test-9-for-humans-do-not-test.patch3.43 KB
human-git-interdiff.txt1.13 KB

OK, here's a patch without that refactoring.

There are two identical foreach ($node_ranks as $node_rank) { lines in the function which makes the diff a bit confusing, so I've created a second version with the first foreach commented out, which should be easier to review. The code is identical, apart from the commented foreach, as shown by the interdiff.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks! I like this version a lot better.

One thing: we don't ever want to use t() in test assertion messages. If you need to inject variables, you can use simple string concatentation, or format_string().

So this use of t() is correct, because it is asserting that some particular translated text appears in the UI:

+    $this->assertText(t('Content ranking'));

But this one is for a message and needs to be removed:

+      $this->assertTrue($this->xpath('//select[@id="edit-node-rank-'.$node_rank.'"]//option[@value="0"]'), t('Select list to prioritise @node_rank for node ranks is visible and set to 0', array('@node_rank' => $node_rank)));

Also, a nitpick: prioritize is spelled with a Z in Drupal-standard US English. :)

Other than those small problems, this patch looks good.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Needs work » Needs review
FileSize
2132647-9-11-interdiff.txt2.34 KB
2132647-nodesearch-settings-test-11.patch3.28 KB

Thanks for the review. Here's an updated patch with those corrections.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Great!

Sorry, I missed this in the last review -- a small coding standards issue: we want a space around . for string concat operator:

+      $edit['node_rank_'.$node_rank] = 10;

should be

+      $edit['node_rank_' . $node_rank] = 10;

There are other spots as well with this same problem. Quite a few, actually.

Thanks!

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
FileSize
2132647-nodesearch-settings-test-13.patch3.3 KB
2132647-11-13-interdiff.txt1.48 KB

This patch has more spaces, I've also added a full stop at the end of the messages. I clicked the wrong button so the interdiff is actually a diff of the patches, but in this case that's probably slightly easier to read anyway.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: 2132647-nodesearch-settings-test-13.patch, failed testing.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

13: 2132647-nodesearch-settings-test-13.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks! This patch looks like a good update of the node rankings test, so that we have a test that uses the UI rather than just the config/variables. Let's get it in.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

13: 2132647-nodesearch-settings-test-13.patch queued for re-testing.

webchick’s picture

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

Yay tests! :)

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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