Problem/Motivation

The patch that was committed caused 11 test fails in Drupal\views\Tests\QueryGroupByTest.

views.view.test_group_by_count_multicardinality.yml has some complex aggregation and sorting needs.

Tests pass on PostgreSQL and SQLite.

Proposed resolution

Add sort fields to new test view.

Remaining tasks

  • Commit patch drupal-2483903-fix-multicardinality-test-7.patch

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Major
Prioritized changes PostgreSQL
Disruption None
CommentFileSizeAuthor
#8 views.view_.blah_.yml6.89 KBmradcliffe
#7 drupal-2483903-fix-multicardinality-test-7.patch1.93 KBmradcliffe
#6 drupal-2483903-fix-multicardinality-test-6.patch151.51 KBmradcliffe
#6 drupal-2483903-fix-typos-in-multicardinality-test-view-6.patch1.31 KBmradcliffe
queries.txt2.36 KBmradcliffe
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner for Drupal Association commented

Mh, those queries don't have a sort criteria at all ... I guess simply adding one would fix it?

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented
Issue summary: View changes

You're right, @dawehner.

I was confused by the patch in the commit and was only looking at the test views that were in the patch. I don't see any sorts defined in views.view.test_group_by_count.yml and views.view.test_group_by_in_filters.yml and views.view.test_aggregate_count.yml

Fixed my assumption in issue summary. Hopefully that makes this patch trivial, and I should be able to write a patch before I get to LA.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner for Drupal Association commented

@mradcliffe
Do you see a chance to get a green run in LA?

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented

Yes, for this task.

For everything, it's a bit hairy because we need to switch from drupalci_testbot legacy to production, and there are issues there to fix related to drupalci. Also need to review and decide on the case insensitive issue (citext vs lower()).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

AFAIK every single DA tech person will be in LA, along with jthorson, so I'm betting you can get some drupalci help. :)

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented
Issue summary: View changes
Status: Active » Needs review
FileSize
drupal-2483903-fix-typos-in-multicardinality-test-view-6.patch1.31 KB
drupal-2483903-fix-multicardinality-test-6.patch151.51 KB

Sigh... I always do everything backwards in my life, code, and art...

1st patch fixes some typos from the original commit in the multicardinality test view.
2nd patch (See comment #7) contains the 1st patch and adds some sorts.

I'm a bit torn on this approach, which I'll relate to you in a poem because I'm tired:

Should we add sorts to the view
to emulate what a real person would do
or always add an order by
which may make a site builder sigh?

In other words I think the value of the test is not as great if we're testing order and expect the user to do themselves, but the only alternative is to add aggregate order by whenever an aggregate is added into the view. The latter could have performance impact.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented
FileSize
drupal-2483903-fix-multicardinality-test-7.patch1.93 KB

And i'm tired, and didn't watch my git diff :(

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented
FileSize
views.view_.blah_.yml6.89 KB

I also did a test in the UI with nodes with the attached view.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner for Drupal Association commented 
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_group_by_count_multicardinality.yml
@@ -34,7 +34,6 @@ display:
           table: entity_test_mul_property_data
-          plugin_id: field
           entity_type: entity_test_mul

why this change ... ah I see duplication.

Should we add sorts to the view
to emulate what a real person would do
or always add an order by
which may make a site builder sigh?

Ha!

-100 to add implicit behaviour, I think people should have a look at PIP 8. In reality the created field is added by default to node based views, so it should be less of a problem,
especially because unless tests, timestamps are in proper order. Yeah entropy in the universe!

In other words I think the value of the test is not as great if we're testing order and expect the user to do themselves, but the only alternative is to add aggregate order by whenever an aggregate is added into the view. The latter could have performance impact.

Well, people should have a look at their views, when they build sites, and I think you always build your sites against a certain target database, given that you can add it if needed.
You know for mysql it seems to work fine, so too bad pgsql people have maybe to do more, but adding an additional sort criteria is probably one of the least problematics steps.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented
Issue summary: View changes

Well, people should have a look at their views, when they build sites, and I think you always build your sites against a certain target database, given that you can add it if needed.

I woke up this morning with the same thought, and I agree.

I ran QueryGroupByTest in sqlite and it passes as well, but a Views test run on sqlite and pgsql drupalci legacy testbots would be appreciated.

Edit: I think I can spin up livetestbot-drupal-latest.iso on the drupalci legacy branch, but i don't have any of the old containers built. It'll take a while.

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented

I did a PG bot run and we have pass with patch applied!

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented

I probably won't be able to get the legacy tesbot up and running on my local box again until this evening for a partial SQLite run.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented

Okay, this passes SQlite too.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I'll put this to RTBC based on #9 and #11.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed b183aa1 and pushed to 8.0.x. Thanks!

  • alexpott committed b183aa1 on 8.0.x 
    Issue #2483903 by mradcliffe, dawehner: Fix tests broken by leverage...

Status: Fixed » Closed (fixed)

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