search

amitaibu / og

A fork to work on OG8
https://github.com/Gizra/og
29 stars 16 forks source link

Clean up and clarify the OgAccess unit tests #230

Closed pfrenssen closed 8 years ago

pfrenssen commented 8 years ago

The OgAccess unit tests are sparsely documented which makes them very confusing to work with. Also it has a property which is very vaguely named $entity, but it is not clear when looking at the tests whether this is a group, or group content, or any other entity.

Let's add some documentation and do some general spring cleaning.

pfrenssen commented 8 years ago

This is still in progress since it builds on top of #226.

pfrenssen commented 8 years ago

Great, #226 is in so this is unblocked!

damiankloip commented 8 years ago

This also conflicts alot with my PR. Seems like this can totally wait? On 10 Jun 2016 17:55, "Pieter Frenssen" notifications@github.com wrote:

This is still in progress since it builds on top of #226 https://github.com/amitaibu/og/pull/226.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/amitaibu/og/pull/230#issuecomment-225236843, or mute the thread https://github.com/notifications/unsubscribe/ABAUw3030wSDnj0eUrOXlXRw1dZ1c1Duks5qKZb1gaJpZM4IzIjQ .

damiankloip commented 8 years ago

@pfrenssen cut me a break, https://github.com/amitaibu/og/pull/225 should at least get a look in first, after https://github.com/amitaibu/og/pull/226 was merged :)

pfrenssen commented 8 years ago

I'm relentless! :D

No this is obviously low priority, let's not have it complicate more important PRs.

pfrenssen commented 8 years ago

225 is in, as far as I know this will not impact anything else any more. I merged in the latest changes from 8.x-1.x.

pfrenssen commented 8 years ago

Thanks for the review. I am going to have a look at this again later. I have other fish to fry at the moment :)

pfrenssen commented 8 years ago

Found the time to address this. Tests will now probably fail due to https://www.drupal.org/node/2752315

pfrenssen commented 8 years ago

This is becoming like a game of whack-a-mole, issue #2752315: Fix visibility of AssertLegacyTrait::assertNoEscaped() is now fixed, and just a few hours later a similar issue popped up: issue #2757139: Fix visibility of AssertLegacyTrait::buildXPathQuery().

I have posted a fix in that issue, if by tomorrow it is not accepted I will make a workaround for us so we can continue working on OG.

pfrenssen commented 8 years ago

The bug in core is fixed! I have restarted the test and now everything is green. This is ready for review again!

amitaibu commented 8 years ago

Thanks!