SQLite: Fix system\Tests\Database\FetchTest [#2475177]

This issue is part of #2454513: [meta] Make Drupal 8 work with SQLite.

Drupal\system\Tests\Database\FetchTest is currently failing on SQLite because Drupal\Core\Database\StatementPrefetch does not take $allowRowCount into consideration.

Comment	File	Size	Author
#1	2475177.patch	801 bytes	amateescu
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

amateescu CreditAttribution: amateescu for Drupal Association commented 21 April 2015 at 13:15

Status:

Active

» Needs review

File	Size
2475177.patch	801 bytes

Test run before:

Test summary
------------

Drupal\system\Tests\Database\FetchTest                        40 passes   1 fails                            

Test run duration: 4 sec

Test run after:

Test summary
------------

Drupal\system\Tests\Database\FetchTest                        41 passes                                      

Test run duration: 4 sec

Comment #2

amateescu CreditAttribution: amateescu for Drupal Association commented 21 April 2015 at 13:37

This patch also fixes Drupal\system\Tests\Database\SelectComplexTest which has the same assertion at the bottom of the test class: http://cgit.drupalcode.org/drupal/tree/core/modules/system/src/Tests/Dat...

Comment #3

m4olivei

he/him

English

Grimsby, ON

CreditAttribution: m4olivei commented 22 April 2015 at 02:27

Confirmed that the patch applies cleanly on the latest D8. It also fixes the failing test and makes sense from a read through. I'd suggest leaving the comment that reads Implementations of StatementInterface., and leaving out the {@inheritdoc} docblock. This whole file could use some documentation love and it probably makes more sense to do it as a whole, rather than piecemeal. I could be wrong, I'm new here.

Comment #4

amateescu CreditAttribution: amateescu for Drupal Association commented 22 April 2015 at 11:48

I think fixing the doxygen block of the function we're dealing with is in scope of this patch, and, yes, we need a different issue to fix the rest of the file.

Comment #5

dawehner

German

CreditAttribution: dawehner for Drupal Association commented 27 April 2015 at 05:59

Status:

Needs review

» Reviewed & tested by the community

I think fixing the doxygen block of the function we're dealing with is in scope of this patch, and, yes, we need a different issue to fix the rest of the file.

Yeah I think its fine, its just a small change and the diff context is still the same / did not got extended.

yes, we need a different issue to fix the rest of the file.

Let's create a follow up for that: #2478355: Fix documentation style in core/lib/Drupal/Core/Database/StatementPrefetch.php

Comment #6

27 April 2015 at 08:02

xjm committed 4f69708 on 8.0.x

Issue #2475177 by amateescu: SQLite: Fix system\Tests\Database\FetchTest

Comment #7

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 27 April 2015 at 08:02

Priority:	Normal	» Major
Status:	Reviewed & tested by the community	» Fixed
Issue tags:		+sqlite

@m4olivei, you are actually correct by the way. :) And If the patch were bigger or the docblock updated were in a different hunk, I'd indeed push back on the coding standards update. But in this case I'm just going to commit it because in an 800-byte patch and the same hunk I'd prefer to just get this in, especially as it's a part of a critical.

I can't decide whether it's major as a test failure on a secondary environment or critical as a required part of the critical, but landing on major at least, and as a simple bug fix, it is prioritized during the beta. Committed and pushed to 8.0.x. Thanks @amateescu, @m4olivei, and @dawehner!