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.

Sign up for GitHub

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

Bus should be a property not a field #432

Merged
merged 4 commits into from
Nov 14, 2018
Merged

Bus should be a property not a field #432

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f5bbad3
Bus should be a property not a field
AnthonySteele Nov 13, 2018
0c4ec92
This is still terrible code
AnthonySteele Nov 14, 2018
100df4d
Lesser hack
AnthonySteele Nov 14, 2018
d1f2e74
Same for integration tests
AnthonySteele Nov 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions JustSaying.UnitTests/JustSayingFluentlyTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,12 @@ protected override JustSaying.JustSayingFluently CreateSystemUnderTest()
// ToDo: Must do better!!
private void ConfigureNotificationStackMock(JustSaying.JustSayingFluently fns)
{
var constructedStack = (JustSaying.JustSayingBus)fns.Bus;

Bus = Substitute.For<IAmJustSaying>();

var notificationStackField = fns.GetType().GetField("Bus", BindingFlags.Instance | BindingFlags.NonPublic);

var constructedStack = (JustSaying.JustSayingBus)notificationStackField.GetValue(fns);

Bus.Config.Returns(constructedStack.Config);

notificationStackField.SetValue(fns, Bus);
fns.Bus = Bus;
}

private void ConfigureAmazonQueueCreator(JustSaying.JustSayingFluently fns)
Expand Down
2 changes: 1 addition & 1 deletion JustSaying/JustSayingFluently.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class JustSayingFluently : ISubscriberIntoQueue, IHaveFulfilledSubscripti
private readonly ILogger _log;
private readonly IVerifyAmazonQueues _amazonQueueCreator;
private readonly IAwsClientFactoryProxy _awsClientFactoryProxy;
protected readonly IAmJustSaying Bus;
protected internal IAmJustSaying Bus { get; set; }
private SqsReadConfiguration _subscriptionConfig = new SqsReadConfiguration(SubscriptionType.ToTopic);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get away with internal and use InternalsVisibleTo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the test isn't super important and just belt-and-braces, we should remove it. It's a self-documented cludge, so if it's not adding much value we should just 🗑 it if it's causing us trouble here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test looks like it's a base class for a lot of things :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bah. A super-cludge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the theme in general with a lot of the tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protected internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant for the whole property, not just the setter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well TIL. https://stackoverflow.com/a/1063920/2225808
You could have a method that sets it, and is internal. 🤔 🤢

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, protected internal for the property is what we want

private IMessageSerialisationFactory _serialisationFactory;
private Func<INamingStrategy> _busNamingStrategyFunc;
Expand Down