Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluate Truth assertion framework #1037

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dmitry-timofeev
Copy link
Contributor

@dmitry-timofeev dmitry-timofeev commented Jul 22, 2019

Overview

The PR is better viewed commit-by-commit. It starts with a singe test migrated to Truth, adds a minimal Subject (~ Hamcrest Matcher), then extends that subject with a new operation, and finally adds some assertions on common library types like Strings and Throwables and various collections.


See:

Definition of Done

  • There are no TODOs left in the code
  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has Javadoc
  • Method preconditions are checked and documented in the Javadoc of the method
  • Changelog is updated if needed (in case of notable or breaking changes)
  • The continuous integration build passes

It is roughly equivalent to a single Hamcrest matcher,
implemented with FeatureMatcher. At this point it might seem more
verbose, but given this class, it seems easier to add new operations —
one does not need the new FeatureMatchers, just new operations like
`hasUrl()`.
It required just a single method 'hasUrlQueryParameter'.

A possible alternative is to implement a subject for HttpUrl,
but we do not seem to make any assertions on this object except
the query parameter from the recorded request.
@dmitry-timofeev dmitry-timofeev added the experimental ⚖️ Experimental PRs usually shall not be merged — they show some approach to get feedback from the team label Jul 22, 2019
@bullet-tooth
Copy link
Contributor

Looks nice.
Will it bring benefits for testing our collections?
For example, is it hard to make such assertions (or similar):

assertThat(listIndexProxy).isEmpty();
assertThat(listIndexProxy).hasSize(1);
assertThat(listIndexProxy).containsExactly("one", "two");

Copy link
Contributor

@MakarovS MakarovS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truth is nice, as is AssertJ. Error messages are almost the same IMO. Subjects seem easy to implement if needed. I think we could go with whichever, in simple cases (so most of the time) it wouldn't matter that much.

@dmitry-timofeev
Copy link
Contributor Author

Thanks for the review,

Will it bring benefits for testing our collections?

The good thing is, IterableSubject is already applicable to our List and Set indexes, and it is non-final, therefore, can be extended to add some specific assertions (proofs, hashes, etc.). The assertions you've seen in the PR for Lists use IterableSubject (= there is no ListSubject in Truth).

Regarding Maps, I currently see several ways how to do that:

  1. A full-blown implementation of a custom Subject for our Map indexes (which are not Maps from the Java Collection Framework).
    • For a proxy of the efforts, required to implement them, one can see the appropriate Truth subjects: for example, MapSubject.
  2. Use a hack and rely on the MapSubject. It can be done in several ways:
    • either by having an explicit method converting MapIndex to a Map: assertThat(exonumMapIndex).asMap().containsEntry(k, v);
    • or by extending the MapSubject and performing this conversion in the constructor: assertThat(exonumMapIndex).containsEntry(k, v).

The big advantage of the second approach is that there is way less operations to implement (test, document) and maintain. There are a couple of disadvantages too: (a) unless we create an MapIndexToMapAdapter, they will always convert the MapIndex into Map in O(N) even for assertions using O(1) operations; (b) MapIndex is not a Map, therefore, some operations have different signatures (e.g., size -> long instead of int). I think (a) is negligible in usual tests; (b), though a valid concern and must be evaluated more carefully by inspecting operations in the MapSubject and Map indexes, does not seem to be a big issue with the present interface of our map indexes being very similar to Maps.

@bullet-tooth
Copy link
Contributor

Looks nice.

they will always convert the MapIndex into Map in O(N) even for assertions using O(1) operations

I believe it's not a problem because usually there're not so many elements in test collections.

@dmitry-timofeev
Copy link
Contributor Author

Exactly, that's why I considered the overhead as "negligible" 🙃

@dmitry-timofeev
Copy link
Contributor Author

There is something for a Testkit Px for sure.

@dmitry-timofeev
Copy link
Contributor Author

In AssertJ, one has to pass Map.Entry instances, which is less
convenient:
```
assertThat(map).containsExactly(entry("hi", 1), ...,
    entry("service", 5))
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental ⚖️ Experimental PRs usually shall not be merged — they show some approach to get feedback from the team
Development

Successfully merging this pull request may close these issues.

3 participants