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.

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

Integration tests #381

Merged
merged 27 commits into from
Aug 30, 2018
Merged

Integration tests #381

merged 27 commits into from
Aug 30, 2018

Conversation

martincostello
Copy link
Member

Apologies in advance for the size of the diff.

This PR does quite a lot of refactoring to support running the integration tests in CI using goaws.

It also in theory supports running them against a real AWS account, but I haven't tested that yet.

A lot of the refactoring relates to how the region, logger factory etc. usage was spread all over the tests, making it hard to centralise into a single place to reconfigure. I've shifted a lot of things around to try and make it more-or-less changeable in a single place. This would allow, for example, a developer in Australia to run the integration tests against a real AWS account with ap-southeast-2 as the primary region instead of eu-west-1.

I've also, where possible, hooked the logger factory into xunit's ITestOutputHelper so the logs get emitted into the test results, so you can read them if the tests fail. At a later point this could be extended into the uni tests as well.

There were also issues with tests hanging (see #380), though I've worked around these issues with usage of ConfigureAwait(false) in the paths for constructors that do async things.

Further, I've simplified the test project dependencies to leverage transient dependencies and reduce duplication, as well as target netcoreapp2.1.

Relates to #159.

Ignore any UpgradeLog*.htm files from Visual Studio.
Sort entries.
Add missing [Collection] attributes.
Simplify test dependencies by removing NLog and using transient dependencies better.
Update to target netcoreapp2.1 in the tests and update the test dependencies.
Remove NLog.
Add infrastructure to allow routing all logging through xunit so when tests fail we will get the log output associated with the test.
Suggest that the integrator can use NullLoggerFactory.Instance from Microsoft.Extensions.Logging.Abstractions.
Shuffle around the references to reduce duplication.
Add the Build.ps1 file added by #377 to the .sln file.
Fix integration tests that have broken over time.
Enable some skip tests that either run quickly with the local simulator or aren't skipped for a valid reason anymore.
Also refactored some of the tests to be clearer to read.
Use ITestOutputHelper where relevant.
Remove redundant test trait (it's the only test with it).
Refactor the tests' configuration to support changing the regions, keys etc. in a central place.
For example, a developer in Australia may wish to configure the tests locally to always point to ap-southeast-2.
Also refactors and renames some stub types to reduce duplication and confusion.
Move some classes to their own files.
Add configuration files for xunit.
Further test refactoring for configurability.
Make some tests async.
Support ITestOutputHelper in some tests where supported.
Move some types into their own class files.
Remove specific RegionEndpoint field values from integration tests.
Remove hard-coded URL to the simulator.
Fix integration tests that hang by using ConfigureAwait(false).
Fix test failing due to a wait no longer being used.
Fix a test failing with the simulator as it was using AWS itself.
Create a fixture that can be used to do various repeated tasks throughout the integration tests.
Add some more usages of ITestOutputHelper.
Tweak some integration tests to try and fix flakiness.
The flaky tests all seem to be related to multiple regions.
Add [Fat] and [Theory] attributes to the integration tests that mark them as skipped if there is no simulator configured or any AWS credentials.
Run the integration tests in the build scripts.
Run the goaws Docker image in AppVeyor and Travis CI to run the integration tests against the SNS/SQS simulator.
@martincostello
Copy link
Member Author

Looks like Linux containers aren't supported in the free version of AppVeyor 😞

Try and prevent queue collisions by using unique names.
Improve the assert messages in the multi-region tests so we can see which region is failing the asserts.
The simulator does not really understand regions, so disable two tests that are specific to them, as they don't reliably pass as despite the published region being changed it might be "received" by a different region.
Copy link
Member

@slang25 slang25 left a comment

Choose a reason for hiding this comment

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

This is looking great!

````c#
var loggerFactory = new LoggerFactory();
ILoggerFactory loggerFactory = NullLoggerFactory.Instance;
Copy link
Member

Choose a reason for hiding this comment

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

TIL 👍

@slang25
Copy link
Member

slang25 commented Aug 19, 2018

Yeah, the deadlocks before were due to the blocking on async as you expected. It was due to xunit enforcing a test concurrency limit, this introduced a SynchronizationContext that would deadlock under certain scenarios, often the tests would pass in isolation, but a full run would almost never work.

The work I did in JustBehave was primarily a pre-req for sorting out these tests, that said, I would like to see it gone from JustSaying completely, for many reasons, including the tests get ran in the constructor of the test classes, which is crazy.

On goaws vs localstack. I got about 80% of tests running with localstack, cause it also emulates iam etc... That said, goaws looks much nicer, and I'd be far more likely to PR to that than localstack. Maybe we could add full support for FilterPolicy for example (although I expect most will just use exact match).

This PR rocks, let me know if I can help.

@slang25
Copy link
Member

slang25 commented Aug 19, 2018

Shame about AppVeyor and linux images on docker (related support question), can we do it with AppVeyor linux build agent images or Travis?

@martincostello
Copy link
Member Author

My initial thinking yesterday was maybe we could run goaws in Travis (it's working now 🎉) and once I've done some local testing with it, we could setup AppVeyor to do the "real" integration tests as a "workaround".

Then we've got PRs testing both with the local emulator on Linux and against real SNS and SQS on Windows?

It's not ideal that we've got different coverage on different OSs, but it would spread things around, at least for an initial merge.

My initial aim was to get this at least green and in a decent working state for a jumping off point for merging to master, then we could do any further improvements on top of that (extending coverage, maybe removing JustBehave etc.).

Use Use MartinCostello.Logging.XUnit to redirect logging output to xunit instead of embedding the code itself.
Update to latest alpha of MartinCostello.Logging.XUnit.
Make integration tests opt-in on Windows while they're not working in AppVeyor.
Do not start the Docker container for the integration tests if they aren't enabled.
@martincostello
Copy link
Member Author

Fun fact: dynamically changing the build Id in the build makes the pending GitHub status for AppVeyor 404:

Expected: https://ci.appveyor.com/project/justeattech/justsaying/build/0.0.386-utfjxwpu
Actual: https://ci.appveyor.com/project/justeattech/justsaying/build/6.0.0-beta2-build386-d5lx2cw2

@martincostello martincostello changed the title [WIP] Integration tests Integration tests Aug 30, 2018
@martincostello
Copy link
Member Author

I've got far enough along in this PR I think that there's enough value there to merge it now even though the overall task in #159 isn't yet complete.

Merging this adds integration tests with goaws for Travis CI, plus locally via opt-in in Build.ps1 if you have Docker installed for Linux containers.

In a follow-up PR I'll do work to ensure we also support running the same test suite against AWS itself if the appropriate environment variables for keys etc. are configured.

@martincostello
Copy link
Member Author

This also gets enough of the refactoring into master to allow others to work on #380.

@martincostello martincostello merged commit db087af into justeattakeaway:master Aug 30, 2018
@martincostello martincostello deleted the Integration-Tests branch August 30, 2018 10:50
@slang25
Copy link
Member

slang25 commented Aug 30, 2018

This is amazing! 🎉 I bought Paul a coffee to say thanks for goaws

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants