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

Do not assert on sort order of equal elements #12778

Closed
wants to merge 1 commit into from

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Apr 28, 2018

Three tests in Piwik\Tests\Unit\DataAccess\JoinGeneratorTest fail on PHP 7.

PHP 7 contained a change to uasort() and friends that may result in different sorting of values that compare as equal.

The three tests all sort three tables where the two of them do not depend on each other and may thus be returned in order. Hence we should not make assertions on their relative order.

@sgiehl
Copy link
Member

sgiehl commented Apr 28, 2018

Not sure if changing the tests is the right approach in this case. Maybe it would be better to change the JoinGenerator to always return the same order accross all php versions

@tsteur any opinion on that?

@tsteur
Copy link
Member

tsteur commented Apr 29, 2018

Yes definitely important to change the JoinGenerator to always return the same set. It is very critical to test the full response here as otherwise generated queries will fail.

@sgiehl
Copy link
Member

sgiehl commented Apr 29, 2018

@c960657 I will close the PR. Thanks for it nevertheless.
Maybe you are keen on changing the JoinGenerator to always return the same set?

@sgiehl sgiehl closed this Apr 29, 2018
@c960657
Copy link
Contributor Author

c960657 commented Apr 29, 2018

Sure, I'd be happy to make a new PR. I just need to understand your suggested approach.

I can change the sort to be stable. If I understand correctly, the point of this is just to make testing easier, not because it matters that the joins are sorted differently in PHP 5 and PHP 7, right?

@sgiehl
Copy link
Member

sgiehl commented Apr 29, 2018

I would say the current difference in order between php version does not cause any known problems, so the target would be to make the returned order consistent (and adjust the tests accordingly). But maybe @tsteur is able to say more.

@tsteur
Copy link
Member

tsteur commented Apr 29, 2018

I would say the current difference in order between php version does not cause any known problems,

Exactly.

so the target would be to make the returned order consistent (and adjust the tests accordingly).

that would be good.
So instead of returning 0 in a usort, we would need to search for the index of a key or item within the original array and check for example whose index is lower or higher and sort it based on the position within the array. Just an example.

@c960657
Copy link
Contributor Author

c960657 commented May 31, 2018

I have filed PR #13025 that implements the suggested stable sort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants