notToThrow Has Ambiguous Semantics for the Not-So-Mindful Reader

[jGleitz]

Platform (all, jvm, js): all Extension (none, kotlin 1.3): none

Code related feature

expect { checkNotNull(value) }.notToThrow {
    toBe("foo")
}

Is your feature request related to a problem? Please describe. notToThrow allows us to specify that no exception should be thrown and then continue to specify expectations about the subject. When reading code using it (yes, code that was written by my younger self), I find this ambiguous. The reason is that toThrow takes expectations about the exception in its body. Hence, my mind is inclined to assume that notToThrow also takes expectations about the exception. I wrongly assume that the semantics are ‘not to throw an exception matching the expectations of the block’.

I know that my wrong assumption makes no sense from a type perspective (unless my lambda returns an exception), however, I don’t think that far at first glance.

Describe the solution you'd like I think we can help readers by changing the name. For example, if the expectation was called notToThrowAndResult or notToThrowAndReturnValue, my mind would recognize that the following expectations are not about the exception but about the subject.

Describe alternatives you've considered Leave the name as it is and trust that others are not as easily fooled as I am.

---

Note from @robstoll Please react with :+1: if you would like that notToThrow gets refactored and state your wishes as comment below. The more upvotes the more likely I will re-consider the current behaviour.

[jGleitz]

The proposed solution would, of course, look a little awkward when specifying the type of the exception that must not occur:

expect { checkNotNull(value) }.notToThrowAndResult<IllegalStateException>().toBe("foo")

We could force users to write something like

expect { checkNotNull(value) }.notToThrow<IllegalStateException>().andResult.toBe("foo")

pro: the andResult makes it very explicit that we continue with the result of the evaluation contra: it might feel unnecessarily complicated.

[robstoll]

One cannot define the type of the exception which should not occur, so no problem there.

I am not yet convinced we need the renaming but you can try to convince me otherwise. notToThrow was introduced as counterpart of toThrow because people wanted to state that no exception is thrown explicitly. So I guess they have tests which look more or less like the following

it("passing empty list throws ...") {
  expect { do(listOf()) }.toThrow<...>()
}
it("passing non empty list does not throw") {
  expect { do(listOf(1,2)) }.notToThrow()
}

The same could be written as follows (remarkably with less failure hints in case an exception occurs though) which I guess most people do instead.

it("passing empty list throws ...") {
  expect { do(listOf()) }.toThrow<...>()
}
it("passing non empty list does not throw") {
  do(listOf(1,2))
}

Maybe it would make sense to introduce an alias for notToThrow which is actually the real behaviour behind it? In this sense notToThrow would become the alias of invoke() -- notToThrow is nothing more behind the scene:

container.manualFeature("invoke()") { this.invoke() }

So I am wondering, would invoke have been the better fit in your code (in case it is public, could you share a link please).

[jGleitz]

Here’s where I encountered the issue: https://github.com/jGleitz/testfiles/blob/master/base/src/test/kotlin/DefaultTestFilesSpec.kt#L144

It previously read

expect { testFiles.createFile("test") }.notToThrow {
	// check that / \ is not messing up the directory structure
	parent.fileName.matches(Regex(".test with -.- in it."))
}

---

I am aware that notToThrow is basically invoke(). However, I like to use it to make my intentions clear. In the code above, the main check is that creating a file does not throw an exception. If the escaping would not take place, creating the file would throw an exception. From my point of view, writing invoke() would make it less clear that this test works by checking that no exception is thrown.

I also like the version of your example that uses notToThrow better, because it states more clearly how the test works.

In my example, the additional check after notToThrow is an additional check that is only applicable for / or \, as those would not throw an exception. You could make a case for having a separate test case for / and \, and I’d probably grant you that. But I still think it’s an example where notToThrow is a better choice than invoke().

To be honest, I currently can’t think of an example where I’d use invoke(). If I don’t expect an exception, I can just inoke the lambda directly. If I know an exception could occur, but expect it not to, I use notToThrow().

[robstoll]

Thanks for the feedback. I couldn't find an example neither where I would use invoke. So this one is clarified for me as well, we don't need invoke. So back to the renaming request.

I am particularly not convinced of notToThrowAndResult because it makes it look like we deal with a Result. notToThrowAndReturnValue is a bit better but we don't use it for other feature extractors which are based on a method. Therefore I still prefer notToThrow Maybe noToThrowAnd would be a compromise?

[jGleitz]

Hm, I get your points. I gave it some more thought and now I also dislike notToThrowAnd (even though I considered it myself). My reason is that:

1. There would still need to be notToThrow for the case that no further expectations follow.
2. notToThrow would need to return Expect<Unit> because we don’t want two ways to achieve the same thing.
3. It feels unnatural to have to change notToThrow to notToThrowAnd if I want to continue making assertions.

So from my point of view, either we leave notToThrow as it is, or try to improve it differently:

First, I think we should remove the overload of notToThrow that takes an expectationCreator. From my point of view, noToThrow().and { … } is just a better way to express the same intent.

Second, if we want to force this on users, we could return a special type from notToThrow that forces users to write and before being able to continue making assertions.

Third, if we are on it, we could name and more expressively, like andReturnValue.

Those changes would force users to write code that I, personally, find easier to read. However, I don’t want to force my opinion on users for an issue where I am not even sure whether I am the only one having it.

[robstoll]

True, noToThrowAnd would not make sense as outlined with your first point and I guess this is the most common use case.

I don't want to remove the overload which expects an exceptationCreator because this would be inconsistent. Feature extractors always provide both versions.

What we could do is to provide a sophisticated builder in addition which would start with notToThrow (without () like we do for toContain) but IMO there should be other use cases as well because I don't want to introduce twice the same thing. Currently I don't see another use case and therefore I would keep things as they are. But maybe you have something in mind which you would use if we had a builder?

[jGleitz]

Feature extractors always provide both versions.

I guess this sentence is a good explanation for why I am irritated with the current solution: notToThrow should, to my mind, not be a feature extractor. When I write notToThrow, I am talking about the expectation that no exception will occur. That is why I am irritated when more expectations follow, especially in a block: In my mind, no feature extraction has taken place.

My proposed solution would separate the ‘no exception’ expectation from the feature extraction, making the feature extraction an optional, separate, explicit step¹.

All of this is not to say that I am convinced that it is necessary to change notToThrow. I can always continue with and in my code without making things more complicated for other users.

But at least I was now able to properly articulate my issue with the current solution. If I have not convinced you: how about we leave this one open with a needs-votes label and close it in some months if it has not gained any votes (it probably won’t, most tickets don’t)?

¹ I am looking at it from a user’s perspective. In reality, notToThrow will always do the actual extraction since it needs to call the lambda. But while this is true for technical reasons, I, as a user, don’t think about it that way.

[robstoll]

Sometimes issues get a vote but rather seldom. In any case, I have added the label.

I think there is a misconception of expect { ... } -- in the past this was a special construct, not creating an Expect but a builder which one finished with toThrow or notToThrow. Nowadays, this is the same as expect(x), i.e. expect({ ... }) just leaving out the parenthesis which is legal due to the {}. In other words, you define a lambda as subject. I guess you already knew this but I figured I state it nonetheless in case someone else should read it.

So... if notToThrow is not a feature extractor then you would still deal with Expect<() -> R> after the call. Most likely not what you expect... right? Because... this would imply you need to call the lambda twice if you want to extract it which is a no go in case you deal with side effects.

Anyway, there is one thing which I think is not ideal with noToThrow which I only realised after your last comment. After we have transitioned to to + infinitive for expectation functions and start feature extractors (in almost all cases) not with to or notTo, we should have a look at notToThrow again. Oh... yes, it's a feature extractor and it is not supposed to start with notTo :thinking:

[jGleitz]

So... if notToThrow is not a feature extractor then you would still deal with Expect<() -> R> after the call.

Not quite. I don’t think that it’s imperative that every expectation returns an Expect for the old subject. Sometimes it just doesn’t make sense. (As an aside: I also think that toThrow { } (with an expectationCreator) should return Unit. It just doesn’t make sense to continue specifying expectations after toThrow {}).

From my point of view, your argument is an argument in favour of my solution. The user should regard notToThrow as an expectation, not a feature extractor, hence, the return type will be something special. To do the feature extraction (once again, from a semantical, not from a technical standpoint), the user has to call the special feature extractor (like returnValue).

Because... this would imply you need to call the lambda twice if you want to extract it which is a no go in case you deal with side effects.

No, as I said, I would make notToThrow return a special type, which stores the return value internally. Users would have to call returnValue on the return type to get an Expect for the return value:

expect { … }.notToThrow().returnValue.toContain("foo")

Anyway, there is one thing which I think is not ideal with noToThrow which I only realised after your last comment. After we have transitioned to to + infinitive for expectation functions and start feature extractors (in almost all cases) not with to or notTo, we should have a look at notToThrow again. Oh... yes, it's a feature extractor and it is not supposed to start with notTo thinking

I agree. From my point of view, that’s another argument in favour of the notToThrow().returnValue solution.

[robstoll]

Not quite. I don’t think that it’s imperative that every expectation returns an Expect for the old subject.

In general I think it should, changing the subject feels like an undesired side effect to me. It can change its type but behind the scene it should stay the same subject. Otherwise a feature extractor or subject changer need to be used.

Feature extractors on the other hand are allowed to incorporate expectations - at least that the extraction works/does not throw

No, as I said, I would make notToThrow return a special type, which stores the return value internally

If we would go with this approach, then I agree, we would need to store it. It would basically be like the old ThrowableThrown builder.

It just doesn’t make sense to continue specifying expectations after toThrow {}).

toThrow { } first returned Unit but I had a use case which proved to me that I should have stuck to the rule. I don't remember exactly the use case but I think it was something along the line of, x should throw IllegalArgumentException and a second call should throw something like TerminatedException. There are various APIs out there with such a behaviour. Also I don't think it hurts someone which only use one toThrow -- which is most likely the vast majority of cases.

The user should regard notToThrow as an expectation, not a feature extractor, hence, the return type will be something special.

First of all, I kind of agree that most users don't expect a feature extraction. I also think that most users will not define additional expectations after it. GitHubs search does not find any (not even yours unfortunately): https://github.com/search?q=notToThrow+extension%3Akt (I did not even spot one for the other assertion libraries providing a notToThrow as well)

But first of all the rule is that an expectation function returns an Expect and IMO there needs to be a good reason to break it. I am not against making an exception but then we can also:

• not rename notToThrow and declare it is an exception to the rule "functions starting with to/noTo are expectation functions"
• remove the overload expecting the expectationCreator.

Since I guess that most users don't use notToThrow { ... } and not even write expectations after notToThrow() I am currently in favour to keep notToThrow as completion to toThrow (which is a feature extraction as well which breaches the to/notTo-rule as well).

[jGleitz]

Okay, so let me dissect the whole situation a little more. The beginning will be all old news for you, but helps me. In the end I’ll make another bold proposal to change Atrium.

In Atrium’s fluent API, there are two reasons for chaining calls: To change the subject or to state an additional expectation:

Atrium allows you to chain assertions or in other words you only need to write the expect(...) part once and can make several single assertions for the same subject. (README)

chaining to change the subject:

expect(person).age.toBeLessThan(19)

chaining to state an additional expectation:

expect(number).toBeGreaterThan(3).toBeLessThan(10)

The first use—chaining to change the subject—makes sense from a grammatical perspective. The second use—chaining to state an additional expectation—not so much, at least not without an and.

Most of the time, the reader knows instinctively which of the two meanings chained calls have, simply because only one of the two options makes sense. For this issue, my instinct failed me: In my mind, chaining after notToThrow was the first use—changing the subject—, while it was actually the second—stating an additional expectation.

After thinking all of this though, I, personally, think that we should remove the second use. I have these reasons:

• There will always be ambiguities like this one. We want to have expectations that change the subject, simply because it is convenient. But this means we will have ambiguities. Take this example: expect(myPath).hasDirectoryEntry("foo").isWritable(). Does myPath have to be writable or does the entry foo have to be writable? How is the reader supposed to know that without looking it up? (It’s the former, by the way).
• Readers can directly see whether the subject was changed: In a chained call, the subject must have changed.
• Chaining is inferior to using an expectationCreator because it doesn’t allow for non-fail-fast behaviour. We explain this difference to users in the README. This is something I always disliked: We bother users with subtleties early on, instead of having an API that guides them to the better solution¹
• Using an expectationCreator yields code that is easier to read. It forces the code to follow a ‘no more than one expectation per line’ convention (unless someone uses semicolons, which is generally frowned upon anyway). From my point of view, this is good. There are studies that show that we (Westerners) focus on the beginning of lines. The further right some statement is, the less likely we are to take note of it.
• It follows Atrium’s ‘only one way of doing a thing’ convention
• I don’t see a real disadvantage

Concretely, I would force any expectation that does not change the subject to return Unit. Chaining would only be allowed to change the subject.

For example:

Currently allowed	Would be forced to be written as
expect(number).toBeGreaterThan(3).toBeLessThan(10)	expect(number) { toBeGreaterThan(3) toBeLessThan(10) }
expect(myPath).hasDirectoryEntry("foo").isWritable()	expect(myPath) { hasDirectoryEntry("foo") isWritable() }

Notice how in the second example, the ambiguity is resolved.

@robstoll is this a change you would even consider? I know it’s another big, breaking change. If you considered it, I’d open another ticket in atrium-roadmap to think it through.

¹ Note that you can always have a fail-fast expectationCreator, if you like.

[robstoll]

For this issue, my instinct failed me: In my mind, chaining after notToThrow was the first use—changing the subject—, while it was actually the second—stating an additional expectation.

Maybe I get you wrong but it is actually the opposite, notToThrow is a subject change currently.

is this a change you would even consider?

I am always open for discussion because it may reveal other weaknesses and other solutions even if the initial requested wish was in the end not considered. I see a very low chance for this one to be considered, especially because we are going to have a distinction in naming after the to + infinitive transition. notToThrow and toThrow will be the exceptions to this rule. I would rather re-think about renaming those to make it clearer that a subject change applies, next to the expectation. On top of my head one idea:

expect { ... }.invokeToThrow<...>()
expect { ... }.invoke()

more technical but also more honest about what is going on. Would certainly be a thing to re-learn for existing Atrium users but maybe better in the long run.

The second use—chaining to state an additional expectation—not so much, at least not without an and.

I guess it also depends on how you format it and how you read it (in the following it can be seen as a list of things number should uphold):

expect(number)
  .toBeGreaterThan(3)
  .toBeLessThan(10)

That's what I see a lot when someone is using the fail-fast syntax and in such a case it reads OK IMO, no and necessary. I always found it a bit awkward if a subject-change is in-between but yeah... I guess that's primarily personal taste

expect(person)
  .age
  .toBeLessThan(19)

personally I would mix the two styles in such a case and write something like:

expect(person)
  .age {
    toBeLessThan(19).toBeGreaterThan(10)
  }
  .name {
    toStartWith("Rob")
  }

Personally I almost always use a block if I expect multiple things because I want to see all of them in reporting.

---

Coming back to the ambiguity problem, once to + infinitive is established, then in my mind it is clear what the following means:

expect(myPath).toHaveDirectoryEntry("foo").toBeWritable()

I would recommend to format it differently but that's again personal taste. The subject changing version would be:

expect(myPath).resolve("foo").toBeWritable()

[robstoll]

closing as there haven't been any votes so far