search

silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 65 forks source link

MNT Make asset variant unit tests order agnostic #535

Closed emteknetnz closed 1 year ago

emteknetnz commented 1 year ago

Issue https://github.com/silverstripe/silverstripe-framework/issues/10403

Fixes 1) SilverStripe\Assets\Tests\FilenameParsing\FileIDHelperResolutionStrategyTest::testFindVariant with data set #0 (SilverStripe\Assets\FilenameParsing\FileIDHelperResolutionStrategy Object (...), SilverStripe\Assets\FilenameParsing\ParsedFileID Object (...)) Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'Folder/FolderFile.pdf' +'Folder/SubFolder/SubFolderFile.pdf' https://github.com/emteknetnz/silverstripe-installer/actions/runs/3700324160/jobs/6268641143

Possible due to the upgrade of the Flysystem dependency, there is an ordering issue that was attempted to be fixed in https://github.com/silverstripe/silverstripe-assets/commit/349508447b66591671b8930a25cd3fc1f78fb97e#diff-0668b7adbf9e875b66c47d35627b4ed054921175c1c8583e407607e4c4629353R526

From memory this unit test was showing different results on local and CI. This unit test was failing for me locally as well, so I've made it order agnostic.

I've raised a new issue to further investigate what's going on here

GuySartorelli commented 1 year ago

You've linked to a diff for a PR where you say "there is an ordering issue that was attempted to be fixed" - but there's no clear indication in that link, nor comments in the original PR, of what the "ordering issue" is, or what was attempted to fix it.... it seems like there's some context here that I'm missing.

It looks to me like this PR just alters the test to say that the bad behaviour is actually good (in which case IMO it's not a fix at all and should not be merged) - but I might be missing something.

GuySartorelli commented 1 year ago

@emteknetnz Can you please respond about my above comment?

emteknetnz commented 1 year ago

In the diff I linked to, the ordering changed because for reasons unknown, so the unit test was updated to account for that. From memory Sabina was experiencing a different ordering on her local vs CI, so the order was thus unpredictable.

I experienced the opposite ordering again in this PR, so I made the unit test order agnostic

GuySartorelli commented 1 year ago

Ahhhhhh okay. So the test was relying on a specific order, but the underlying functionality is still correct. That's okay then.