Stop using JustBehave

[martincostello]

As part of the investigation for #159 I've found that there's a lot of use of Task.GetAwaiter().GetResult() and Task.Wait() to work around limitations with JustBehave's design of running test setup and teardown steps on synchronous code paths for constructors and disposal.

JustSaying is heavily asynchronous now, so this just profligates poor async usage throughout the tests.

I also think it is also causing deadlocks when I try and run them locally en-masse using the Visual Studio test runner with a local AWS simulator. This is a hunch, but it mainly comes from the fact that if I run all the tests they seem to get stuck, but if I run tests individually they pass.

The pattern of calling derived methods from constructors in JustBehave also prevents ITestOutputHelper (and other things) from being injected into the tests and being used as part of the test setup as the class' own constructor logic runs after the overridden classes, meaning any fields don't have their values set yet.

I'm happy to do the work to refactor this out of the integration test project myself as part of #159 if we agree that it's a necessary step to move the test suite forward.

I think the time has come to put it out to pasture for JustSaying and adopt a more conventional and xunit-friendly test style using fixtures.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[martincostello]

Soon...

[josephwoodward]

You've got my vote 👍

[martincostello]

Man, this is going to be a mission....

[martincostello]

Had a quick hack-around, but it's usage is so pervasive that getting rid of it basically means slowly re-writing all the tests from scratch, deleting the old-style tests as they're switched, and then removing the JustBehave reference at the end once they're all done.

I think we'd all have to tackle a bit each over several weeks to scrub it entirely, short of deleting everything and then starting from scratch, which doesn't seem the wisest idea, though could maybe done slowly in a long-lived branch with the "big bang" happening at the end with it being merged to master in one go.

[shaynevanasperen]

@martincostello Yep, I came to the same conclusion a year ago. Perhaps we should only tackle it as part of the major "re-write" we plan to do.

[martincostello]

Because JustBehave is not strong-named, we can't strong-name JustSaying until we remove it (see #405).

[slang25]

@martincostello How does the library used in a test project affect our ability to use strong-naming?

[shaynevanasperen]

@slang25 Because of InternalsVisibleTo

[slang25]

Damn, did not know that! We could introduce a configuration for "release but not signed", if running the tests in debug isn't desirable. It's a bit grim though 😞

Seems like yet another reason to overhaul the tests. The coverage is inconsistent, JustBehave forces bad async, we can't do strong naming... the list is long 😆

[slang25]

How about this as a short term fix: justeat/JustBehave#30

[martincostello]

"fix" 😉

I think that it's more of a crutch for the strong-naming problem, than a fix for the tests, but it's a start.

[slang25]

Yeah don't get me wrong, the tests suck, this won't fix that. But it will stop blocking and deadlocks in the startup, and the blocking teardown can be removed from the code here. Given its such a huge effort to rewrite this makes things stable for the short term.

[AnthonySteele]

This has been merged and just needs a release now: justeat/JustBehave#31

[martincostello]

Now that #431 has been merged, I'll look to start work (it make take several PRs and some time to do fully) on refactoring the tests over to the new builder API and culling the use of JustBehave as I go.

[martincostello]

Pressed close by accident.

[AnthonySteele]

I had a look around, and I don't see much problematic Task.Wait() or any GetAwaiter in the tests. So things have changed? What remains to do?

if we want a quick way to remove this ref, then we can copy in the source for a few classes, i.e. AsyncBehaviourTestBase, XAsyncBehaviourTest instead, and then refactor from there.

[martincostello]

Porting the tests over to the new style and the fluent API will fix this issue.

I'd rather the focus was put into that, than this issue. Fluent API tests will not benefit from JustBehave being ripped out and replaced by source references, but migrating the tests will allow us to remove JustBehave.

[slang25]

We've sorted most of the big nasties. It would be good to phase out the JustBehave tests because no one who actively works on JustSaying likes JustBehave or the style (please correct me if I'm wrong), and it gives us a chance to opportunistically rationalise the tests (it's hard to see what they are testing at times).

Agreed with @martincostello 💯 Focusing on the Fluent API is the right thing to do.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[martincostello]

General consensus in meeting yesterday with @brainmurphy and @iancooper was that we just delete any tests using JustBehave (I'll do this once #546 is merged) and that "don't conform" so we can push on with trying to get v7 in a releasable state.

Then once #547 is in and we know exactly what is and isn't covered by the remaining tests, we can write new tests to bring the coverage back up to comparable levels before v7 ships.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.