search

ActiveCampaign / postmark-dotnet

A .NET library for the Postmark API
http://developer.postmarkapp.com/
Other
50 stars 46 forks source link

unit test requirements un(der)documented #78

Open hn3000 opened 4 years ago

hn3000 commented 4 years ago

As a potential contributor I cloned this repository and tried to run all unit tests to ensure my setup is working correctly.

This results in massive failure:

Test Run Failed.
Total tests: 85
     Failed: 78
    Skipped: 7
 Total time: 1.7394 Seconds

The README does not mention any specific instructions for running the tests, the wiki it refers to does not mention anything either.

A closer look at the failures reveals that the failures assert that configuration settings are provided which are not included in the repository. A search for the configuration variables does not show any examples or documentation about them in this repository.

An example for a similar configuration can be found in the postmark-php repository, but it does not elucidate on the expectations for the tokens that need to be configured, either.

I can guess, what an WRITE_ACCOUNT_TOKEN is, but what exactly is the required configuration for the READ_SELENIUM_OPEN_TRACKING_TOKEN?

I am also not quite comfortable to give unit tests a valid account token with write permissions, would a mock not make more sense for unit tests? (Or did I guess wrong what that token above should be?)

Could you please provide some more information about the unit tests and how to run them?

Would you be open to a PR for a version of the unit tests that do not require the actual postmark back-end to run?

atheken commented 4 years ago

Hi @hn3000 - Thanks for your interest in contributing.

While mocking and isolating code under test is a standard best practice, we don't believe it is useful for this scenario. The tests in this library are here to ensure the this library properly integrates with the Postmark API, so it's an exception to the general practice.

We use TravisCI to do these integration tests using servers that are pre-populated with data. If you'd like to run the tests locally, these are the necessary env vars, which can also be stored as keys in a file called testing_keys.json in a parent directory of the repo:

https://github.com/wildbit/postmark-dotnet/blob/master/src/Postmark.Tests/ClientBaseFixture.cs#L19-L29

If a readme on how to configure these would be useful, that's something we can add, but we probably won't add any mocks to the tests, as again, this significantly reduces the usefulness of the tests, and increases the overhead in adding new endpoints and tests to this library.

We understand that getting a full test suite to pass locally may be more effort than it's worth, but if you have a specific need with the library, please feel free to open an issue/PR.

We will happily review and potentially accept these patches after we do our own verification/addition of tests, but large-scale refactors are probably unnecessary.

Hopefully this is helpful, but please feel free to share your thoughts or contact us in support if you'd like to more guidance on contributing.

hn3000 commented 4 years ago

A Readme explaining how to configure the environment vars would be very useful.

With regards to mocking, I have had good results with mocked http APIs, so in your case there we would implement very basic mocks of the routes used in the unit tests. That would not make the integration test against the actual service redundant but would allow some coverage for unit testing. If the mocks do some minimal checking (like validation of input against an openapi spec), these tests will catch stupid mistakes and typos without being to annoying.

A good alternative to these mocks would be a testing account on your systems that could be configured to not actually send emails, but I can understand if you're concerned about such an account enabling DOS attacks on your infrastructure.