search

CBielstein / APRSsharp

APRS# - Modern APRS software for the amateur radio community
MIT License
12 stars 5 forks source link

Use `Assert.Throws` for tests expecting an exception #25

Closed CBielstein closed 3 years ago

CBielstein commented 4 years ago

Several tests are using an outdated model of ensuring an exception is thrown. An example is in APaRSerUnitTests.PositionUnitTests.DecodeLatitudeOutOfRange (test/APaRSerUnitTests/PositionUnitTests.cs) where a model of try, catch and return, then throw an exception after the catch is used. This should instead be written with Assert.Throws for a more clean test model.

CBielstein commented 4 years ago

Happens in at least one place in TimestampUnitTests.cs as well.

eddywine commented 4 years ago

Will work on this together with the other 3 PRs after the current reviews

CBielstein commented 4 years ago

@eddywine This is a low priority issue that I just opened so that we don't forget about it but it doesn't really change functionality. I'd rather have you working on new features instead.

We're currently working on finishing up Milestone 1: Clean up and unify old code bases. Almost done there (all of the work is either in progress or assigned). After that, I'd rather you move on to decoding on the command line and then APRS-IS integration.

dk-obrien commented 3 years ago

I can fix these up for you if you don't have anyone else working on them.

CBielstein commented 3 years ago

@dk-obrien That would be fantastic!

Admittedly, these tests are pretty dated and written when I was less skilled at testing. haha Several are catching NotImplementedExceptions which is not great...but it is what it is for now. If you decide to take on this issue, please don't feel you have to address the shortcomings in the tests aside from just replacing the current logic of:

try
{
    // Some code we expect to throw
}
catch (Exception ex) // Or more specific type we expect to throw
{
    // Some other validation
}

Assert.True(false); // Or some other way of failing

Can simply be replaced with something along the lines of

Assert.Throws<Exception /* or more specific exception */>(() => /* Code we expect to throw */);

Please feel free to ask any questions you have!

dk-obrien commented 3 years ago

Here you are. I left the functionality of the test that I found that was catching the NotImplementedException the same so that's going to swallow that Exception until you remove it from your codebase. I hope this is what you intended.

CBielstein commented 3 years ago

@dk-obrien Looks great! I've approved the PR. Thanks so much for making the change. :+1:

I'll add it to my list in the next week or so to try and identify some more low-hanging fruit in this repository just in case you're looking to make more contributions somewhere. :smile: