spectre-team / spectre

This project provides an Spectre's implementation of Data Mining Group's algorithms for processing mass spectrometry imaging (MSI) datasets.
https://spectre-team.github.io/
Apache License 2.0
2 stars 1 forks source link

Read spectra by coords #123

Closed mg6 closed 7 years ago

mg6 commented 7 years ago
mg6 commented 7 years ago

Get Spectrum by location (API)

mg6 commented 7 years ago

@Daroslaw Agreed that we should provide messages for Asserts but, in my opinion, only when the assert method used and, consequently, its failure message themselves do not convey the intent clearly. Please consider the following examples:

// 1

var spectrum = _controller.Get(...);
Assert.Fail();  // this is not enough information

var spectrum = _controller.Get(...);
Assert.Fail("Should throw for invalid coords");  // now the assert’s purpose is clear
// 2

Assert.NotNull(spectrum); // clear intent, DRY
Assert.IsInstanceOf<Spectrum>(spectrum); // same as above

Assert.NotNull(spectrum, "Should return a non-null value");
    // redundant, already expressed with assert method
Assert.IsInstanceOf<Spectrum>(spectrum, "Should be of spectrum type");
    // same as above
// 3

catch (HttpResponseException e)
{
    Assert.AreEqual(actual: e.Response.StatusCode, expected: HttpStatusCode.NotFound);
        // comparison on enumerated values, clear intent
}

catch (HttpResponseException e)
{
    Assert.AreEqual(
        actual: e.Response.StatusCode,
        expected: HttpStatusCode.NotFound,
        message: "Should respond with proper HTTP code");  // feels redundant in my opinion
} 

catch (HttpResponseException e)
{
    Assert.AreEqual(
        actual: e.Response.StatusCode,
        expected: HttpStatusCode.NotFound,
        message: "Should respond with 404 Not Found code");
            // response code repeated twice, too specific
} 

I think the second case in // 3 is disputable but will add it to address your concern.

gmrukwa commented 7 years ago

@mg6 in my opinion, when we have small tests (single assert or atomic functionality), name of the test should describe everything. Assertion messages are really important if we insert any greater number of asserts into single test, especially if they do not test the same functionality, which should not occur.

Daroslaw commented 7 years ago

Alrighty then.