Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix onNotSuccessfulTest skipping the latest query #5740

Closed
wants to merge 1 commit into from
Closed

Fix onNotSuccessfulTest skipping the latest query #5740

wants to merge 1 commit into from

Conversation

guilliamxavier
Copy link
Contributor

... + fix SQL queries numbering in test failure output, as doctrine/dbal#2340


The bottom modification in commit 0fa136e (whose effect is still there in https://github.com/doctrine/doctrine2/blob/master/tests/Doctrine/Tests/OrmFunctionalTestCase.php) was supposedly intended to output only the 25 (max) latest queries from the most recent to the oldest (rather than all the queries from the first to the latest), but it introduced a regression:
let N be a shorthand for count($this->_sqlLoggerStack->queries),
given that we're inside the condition that N!=0, i.e. N>0, i.e. N>=1,
the loop for($i = N-1; $i > max(N-25, 0); $i--)
iterates from N-1 (included) down to max(N-25, 0) excluded, so e.g.:

  • if N is 1: it iterates “from 0 (included) down to 0 excluded”, i.e. it doesn't iterate at all!
  • if N is 10: it iterates from 9 (included) down to 0 excluded, i.e. from 9 down to 1;
  • if N is 35: it iterates from 34 (included) down to 10 excluded, i.e. from 34 down to 11;

so it should rather have been iterating from N-1 (included) down to max(N-25, 0) (included),
i.e. the > should have been >= in the loop condition.
... At least until the change doctrine/dbal@8cbb70c#diff-9997e1f6eb43e2dcdf6f284b1d49164e, before which the DebugStack was storing its queries at indices 0 to N-1, but since this change (more than 5 years ago) it has been storing them at indices 1 to N (and there are even tests that depend on that, for example: https://github.com/doctrine/dbal/blob/0d76c6e952bd97f2ccc9232b1e0e64cb79cf5652/tests/Doctrine/Tests/DBAL/Logging/DebugStackTest.php#L22 (and also line 33) and https://github.com/doctrine/doctrine2/blob/97cc49033ee18e2ecb5803f95732ecdc6875aec0/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC1595Test.php#L41),
so now the loop should rather iterate from N (included) down to max(N-25, 0) excluded,
i.e. we should remove the -1 in the initialization of $i (and let the > in the condition), so as to have:

  • if N is 1: it should iterate from 1 (included) down to 0 excluded, i.e. 1 only;
  • if N is 10: it should iterate from 10 (included) down to 0 excluded, i.e. from 10 down to 1;
  • if N is 35: it should iterate from 35 (included) down to 10 excluded, i.e. from 35 down to 11;

and lastly, the loop body should output $i directly for numbering rather than $i+1 (same as PR doctrine/dbal#2340).

(PS: I don't know how to write a unit-test for this fix, as it concerns the debug output for test failures...)

+ fix SQL queries numbering in test failure output, as doctrine/dbal#2340
@guilliamxavier
Copy link
Contributor Author

The Travis CI build failures seem random and unrelated to this change...

@Ocramius Ocramius added this to the 2.6.0 milestone Jun 24, 2017
@Ocramius Ocramius self-assigned this Jun 24, 2017
@Ocramius
Copy link
Member

After trying to rebase this for merging into master, I noticed that #6005 provided a fix for this already: closing.

Thanks for the help and the detailed explanation though, @guilliamxavier!

@Ocramius Ocramius closed this Jun 24, 2017
@guilliamxavier guilliamxavier deleted the patch-4 branch June 29, 2017 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants