attach remark to assertion

[robstoll]

Similar to #9 but this time we like to attach something to an assertion which should always show up.

expect(person) {
  feature { f(it::children) }.all { 
    withRemark({
      feature { f(it:.age) }.isGreaterOrEquals(18)
    }, "the legal age of maturity in Switzerland is 18")
  }
}

Should look like the following in reporting:

expect: Person(...)
-> children
  ◆ all entries: 
      » -> age:
          - is greater or equals: 18
        ℹ the legal age of maturity in Switzerland is 18

Syntax and output to be discussed

[jGleitz]

I also have this use case (atrium#627). However, I would propose a slightly different API:

expect(proposeAFileName()).containsNot("?").because(
   "? is not allowed in file names on Windows"
)

or, in your case:

expect(person) {
  feature { f(it::children) }.all { 
      feature { f(it:.age) }.isGreaterOrEquals(18).because(
          "the legal age of maturity in Switzerland is 18"
      )
   }
}

I think my API is easier to write & read.

I would suggest also adding “because” to the output. I like the “ℹ” in front of it. It’s better than the “🛈” I proposed.

[robstoll]

I agree it is more readable without the {}, the only problem is, it will not be evaluated in case of a failure because the previous statement already throws

[robstoll]

Assuming we would have a compiler plugin and rewrite the your because to a {} version so that it is also reported in case it fails, what statement should be transformed. only the last one? How would you add a hint to f { f(it::age) }... ?

[jGleitz]

the only problem is, it will not be evaluated in case of a failure because the previous statement already throws

uh, I always forget about that 🤦. The how-manyith time is it? This builder-like syntax tricks me every time!

Another possibility would be a syntax like this:

expect(proposeAFileName()).containsNot("?") {
     because("? is not allowed in file names on Windows")
}

This would be easy possible (we would need to evaluate the assertionCreator even for failures, without having a subject, swallowing all exceptions and just remembering the becauses) for any assertion that already takes an assertionCreator. All other assertions would need to be adapted to take an optional block, though. I am not sure whether this is worth the effort. Probably only if we have different use cases like this, where we want a generic way to customize assertions.

Assuming we would have a compiler plugin and rewrite the your because to a {} version so that it is also reported in case it fails, what statement should be transformed. only the last one? How would you add a hint to f { f(it::age) }... ?

I think this is pretty clear: The hint belongs to the Expect that I called because on; i.e. the instance it would have been called on if the assertion did not fail.

[robstoll]

What syntax do you propose for something like:

expect(person) {
  feature { f(it::children) }.all { 
      feature { f(it:.age) }.isGreaterOrEquals(12).isLessThan(18).because(
          "we use the definition that teens are between 12 and 18 years old"
      )
   }
}

Where the remark should be for both isGreaterOrEquals(12).isLessThan(18)? Also I am not sure how this should look like in reporting

[jGleitz]

I did not even know and existed. How does reporting look like for it?

I assume that when using and, both assertions will be reported on a failure, no matter which assertion actually failed. In that case, there is no ambiguity: whether the remark was meant only for the last assertion or for both of them, reporting will look the same either way. Right?

[robstoll]

I removed and from the above example. It's just a filler and does not have any behaviour.

The problem with .because is that you cannot signify a grouping. It is always for the last statement. With {} you could signify, it is for all assertions between the braces:

withRemark({
  isGreaterThan(1)
  isLessThan(10)
}, "remark")

I am not sure how we should present that in reporting though

[robstoll]

We could also have both versions, one which comes after the assertion such as your because and one which is in front and allows grouping such as withRemarkFor. We should also take into consideration how this sums up with failure hints #9

[jGleitz]

However, I am not convinced that having a compiler plugin for Atrium is sensible. What’s your take: Is my proposal above (having a lambda-taking version of all assertions) a possibility we should consider?

---

The problem with .because is that you cannot signify a grouping. […] We could also have both versions, […] we should also take into consideration how this sums up with failure hints

Are you sure that having a remark for multiple assertions is a use case we should support? On the face of it, I feel like it causes difficulties is reporting. Is it worth it? Users could just duplicate messages if necessary. I am not sure that that would be so bad:

expect(person) {
  feature { f(it::children) }.all { 
      feature({ f(it:.age) }) {
            isGreaterOrEquals(12).because(
               "we use the definition that teens are at least 12 years old"
            )
            isLessThan(18).because(
                  "we use the definition that teens are young than 18 years"
           )
      }
   }
}

[robstoll]

However, I am not convinced that having a compiler plugin for Atrium is sensible.

I am also not convinced yet, I just see more and more potential but not worth yet IMO.

Users could just duplicate messages if necessary. I am not sure that that would be so bad

That's true, we could at least start this way and see if we really have the need for the grouping 👍

The problem with because remains, that it is not reported if the assertion fails. But IMO this is OK as remarks serve two purposes:

• write down some docs for others
• add it to reporting for those people which report not only failing assertions but also successful.

The case that we want to add a hint to an assertion in case of a failure is covered by #9
Yet, I am not sure if it is a good idea to have two different syntax for it. The failure hint definitely has to be defined before and not after in order that it can be reported in case the assertion fails. What do you think about that?

[jGleitz]

Yet, I am not sure if it is a good idea to have two different syntax for it.

I am pretty sure that we should not. I currently see no good use case for differentiating between “remark” and “failure hint”.

If I want something that only shows up in the code, I can use a comment. There is no need for an API, from my point of view.
I can also not imagine why I would add a remark for a successful assertion, but have a problem with it being reported for a failing assertion.

So from my point of view, we should have only one mechanism that always shows up, no matter whether the assertions succeeds or not.

[jGleitz]

Back to the syntax: If we don’t want to use my (more involved) approach of

feature { f(it:.age) }.isGreaterOrEquals(18) {
    because("the legal age of maturity in Switzerland is 18")
}

I think this would still be more intuitive than the withRemark syntax:

because("the legal age of maturity in Switzerland is 18") {
    feature { f(it:.age) }.isGreaterOrEquals(18)
}

I think it reads more fluently and the style is more akin to what we are used from the API.

If we assume that the remark should be printed below the assertion, this proposal breaks with atrium’s convention that reporting should reflect writing. I never cared for this convention at all, so I don’t care here either. If you deem it important, I think I’d vote for changing the reporting (i.e. print the reason before the assertion) instead of using the (in my opinion) more clumsy syntax.

[robstoll]

Good that you see it this way, I also think we should not have two different syntax. Attaching remarks might get useful once we have the html reporting. Let's move out discussion to #9. We have examples there (also real world examples). Maybe easier to see what fits and what not. I'll just copy your last comment over.

We can come back to this once we have the html report (don't think this will happen near future though).

[jGleitz]

We can come back to this once we have the html report (don't think this will happen near future though).

I’d suggest closing the ticket until somebody comes around needing it. But I have no strong opinion on that, I just think that less open tickets is usually better.

[robstoll]

So back here again (see excursion in #15).
I think we can conclude that it makes more sense to have the text right after the keyword instead of after the lambda. And I am also fine with using because instead of withRemark
So I would go with:

because("the legal age of maturity in Switzerland is 18") {
    feature { f(it:.age) }.isGreaterOrEquals(18)
}

and with this syntax we can do the grouping if one wants:

because("we use the definition that teens are between 12 and 18 years old") {
    feature { f(it:.age) }.isGreaterOrEquals(12).isLessThan(18)
}

Seeing the whole context:

expect(person) {
  feature { f(it::children) }.all { 
      because("we use the definition that teens are between 12 and 18 years old") {
        feature { f(it:.age) }.isGreaterOrEquals(12).isLessThan(18)
      }
   }
}

I think it reads quite good.

Regarding reporting: maybe it would even make sense to include the because in reporting? As follows (then it makes sense to print it before IMO):

expect: Person(...)
-> children
  ◆ all entries: 
      ℹ because we use the definition that teens are between 12 and 18 years old
        » -> age:
           - is greater or equals: 12
           - is less than: 18

vs

expect: Person(...)
-> children
  ◆ all entries: 
      » -> age:
          - is greater or equals: 12
          - is less than: 18
        ℹ we use the definition that teens are between 12 and 18 years old

What do you think?

[jGleitz]

Regarding reporting: maybe it would even make sense to include the because in reporting?

Yep, I suggested that myself:

I would suggest also adding “because” to the output.

Regarding “above” or “below”: At least in my experience, the standard form of an english sentence containing “because” has “because” at the end. I fear that if we put it at the top, we might create an ambigous situation where it is not entirely clear what the “because” refers to.

From the top of my head, I can think of this—bad—example:

expect: Person(...)
-> children
  ◆ all entries: 
      ℹ because this has to hold everywhere
      ◆ all entries: 
          - is greater or equals: 12

No matter the position, I think we should always include the “because”.

[robstoll]

I would suggest also adding “because” to the output.

Sorry must have overlooked that, good that we came to the same conclusion 👍

At least in my experience, the standard form of an english sentence containing “because” has “because” at the end.

that's true, so let's put it at the end including because

[jGleitz]

So—let’s do it?

[robstoll]

Yes, I'll create an issue and link it here

[robstoll]

@jGleitz I am not yet 100% sure how the reporting should look like. Following a few examples:

single assertion

expect(fileName).because("? is not allowed in file names on Windows"){ containsNot("?") }

report:

expect: "file?Name"     (String <11>)
  ◆ contains not: "?"    (String <12>) 
ℹ because: ? is not allowed in file names on Windows

multiple assertions:

expect(person.age).because("we use the definition that teens are between 12 and 18 years old") {
  isGreaterOrEquals(12)
  isLessThan(18)
}

report:

expect: 10    (Int <11>)
  ◆ is greater or equals: 12    (Int <12>)
  ◆ is less than: 18    (Int <13>) 
ℹ because: we use the definition that teens are between 12 and 18 years old

The indentation looks weird for a single assertion but makes sense for multiple assertions (please neglect that is less than: 18 would not be shown for this particular case). What do you think?

[jGleitz]

I think we should indent the ℹ:

expect: "file?Name"     (String <11>)
  ◆ contains not: "?"    (String <12>) 
  ℹ because: ? is not allowed in file names on Windows

expect: 10    (Int <11>)
  ◆ is greater or equals: 12    (Int <12>)
  ◆ is less than: 18    (Int <13>) 
  ℹ because: we use the definition that teens are between 12 and 18 years old

True, there will be no syntactic way to differentiate between a reason for only the previous assertion and a reason for multiple previous assertions. However, I think this will not be too bad: I expect that in most cases, it will be easy to judge from the content which assertion(s) the reason belongs to.

[robstoll]

We usually don't indent, I should have added one more assertion to make that clear:

expect(person)
  .age { 
    because("we use the definition that teens are between 12 and 18 years old") {
      isGreaterOrEquals(12)
      isLessThan(18)
    }
  }.hairColour.toBe("blond")

Would result in the following with indentation:

expect: Person(...)
◆ > age: 11
      - is greater or equals: 12    (Int <12>)
      - is less than: 18    (Int <13>) 
    ℹ because: we use the definition that teens are between 12 and 18 years old
◆ > hair colour: "black"
    - to be: "blond"

without indentation:

expect: Person(...)
◆ > age: 11
    - is greater or equals: 12    (Int <12>)
    - is less than: 18    (Int <13>) 
    ℹ because: we use the definition that teens are between 12 and 18 years old
◆ > hair colour: "black"
    - to be: "blond"

At least for the above example without indentation is better IMO.

[robstoll]

another example:

expect(p).because("? is not allowed in file names on Windows") {
  startsWith("b")
  endsWith("a")
}.containsNot("c")

with indentation:

expect: "asdf
  ◆ starts with: "b"
  ◆ ends with: "a"
ℹ ? is not allowed in file names on Windows
◆ contains not: "c"

without:

expect: "asdf
◆ starts with: "b"
◆ ends with: "a"
ℹ ? is not allowed in file names on Windows
◆ contains not: "c"

alternatively indent ℹ️ as well:

expect: "asdf
  ◆ starts with: "b"
  ◆ ends with: "a"
  ℹ ? is not allowed in file names on Windows
◆ contains not: "c"

[jGleitz]

Ah, I see, You are asking whether because should start a new indentation level for all items that are affected by it.

I think not. For the following reasons:

1. I think the ℹ should be on the same level as the assertions it belongs to. Everything else just doesn’t make sense for me. If the reason is on level more on the outside, I feel like it does not really belong to the assertions. So I think the ℹ has always to be on the same level as the assertions.

2. If we start with that, indenting the elements does not make that much sense if we have more than one reason:

expect: "asdf
 ◆ starts with: "b"
  ℹ b is important
◆ ends with: "a"
 ℹ a is important

here, the indentation is not helpful at all, so I think we should omit it in the first place.

In your last example above, it feels odd to me, that contains not: "c" is on a more outside level, even though it releates to the same subject. That is another reason not to add indentation for because.

[robstoll]

Good reasons, and to be fair, the last example is quite convoluted, I think the normal case will look perfect without indentation. Thanks for your feedback 👍

[robstoll]

we already have because, closing