search

silverstripe / silverstripe-hybridsessions

Hybrid Cookie / DB Session store for Silverstripe
BSD 3-Clause "New" or "Revised" License
16 stars 21 forks source link

Skip tests that rely on DB if MySQL not being used #38

Closed dhensby closed 6 years ago

dhensby commented 6 years ago

fixes #37

dhensby commented 6 years ago

Interesting, we have test failures against the hybrid store that are not DB driver related - do we have a bug?

NightJar commented 6 years ago

assertEquals is doing strict type checking in PHP 7.2 it would seem. But then that begs the question of if it's === why does it work in other versions?

This could be the issue: https://github.com/dhensby/silverstripe-hybridsessions/blob/14b78dd68a8034b3031f3af7aabc692e6650f036/src/Store/CookieStore.php#L105 Seems to me it should return an empty (and serialised) array, i.e. a string.

I guess the behaviour of unserialise is slightly different in PHP 7.2

dhensby commented 6 years ago

I don't think it's 7.2 vs 7.1 tbh; assertEquals() has always been strict and assertSame is loose.

We unserialize the value so it should come out the same... we aren't try to assert [falsey] == [falsey] in this test, it's that an[array with data] == [array with data]`

NightJar commented 6 years ago

Ah hah.

HybridSession is made to use CookieStore, then fall back to DatabaseStore when the former doesn't cut it.

Our fatal mistake was to assume that the CookieStore tests were also just the AbstractTest with the getStore method implemented. However there are actually four tests for CookieStore, and it overrides the testStoreLargeData test, because it's supposed to fail! CookieStore does not (and cannot) store large information, so the whole idea of this test for HybridSession is to fall back to storing large things in the database.

Removing the database layer breaks that assumption and causes the test to fail.

So again if MySQL isn't present it needs a skip, becasuse no data is never going to suddenly be some data (hur hurr heartblead). As to why it works on php5 and 7.0... :man_shrugging:

robbieaverill commented 6 years ago

These tests aren't very nice anyway. The HybridSession class deserves some actual unit tests I reckon

robbieaverill commented 6 years ago

@dhensby I've refactored the HybridSession unit tests - see what you think, you're more than welcome to reset and wipe out my commits =)

robbieaverill commented 6 years ago

@dhensby I've rebased this branch FYI

dhensby commented 6 years ago

well, we are green - I won't ask too many questions :)