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

Should we allow other testing frameworks in the java track? #1803

Closed
jmrunkle opened this issue Apr 15, 2020 · 19 comments
Closed

Should we allow other testing frameworks in the java track? #1803

jmrunkle opened this issue Apr 15, 2020 · 19 comments

Comments

@jmrunkle
Copy link
Contributor

Expanding this discussion from #1634.

Currently, we mostly only use the generic JUnit asserting framework (eg. org.junit.Assert.*), but we do not have any officially documented policy in our contributing guide about using other testing frameworks. We should either explicitly allow or disallow the other frameworks.

Arguments for using only the generic JUnit Assert methods:

  • We want it to be as easy as possible for people to do these exercises and therefore want to limit the different libraries and syntax they need to understand to use the tests.
  • This is consistent with our policy to only use the Java standard library for our exercises.

Arguments for allowing one or more additional testing frameworks:

  • They provide more detailed errors that make it easier for students to debug their tests.
  • org.junit.Assert methods do not distinguish clearly between the expected and the actual so students have to look at the test code to see what was actually expected rather than getting that information straight from the error itself.
  • Exposure to the larger set of test frameworks is a useful skill for the students to aquire.
  • Any reasonable testing framework should be easy to read even without lots of experience.

If we are going to allow another test framework, I suggest that we should choose one framework to make the experience consistent while they complete exercises. Here are the possible contenders:

In my experience, Hamcrest is the least clear and the most bizarre to write tests with its fuzzy matcher style. As such, I would prefer to ban Hamcrest from the Java track (even though it has already been used in a couple places).

I have most experience with Truth (it is what I use daily at work) and find that the errors and style of writing the tests is most straightforward (for example, an IDE will suggest helpful autocompletions). This is my preferred option.

Looking at AssertJ, it seems to also have a very reasonable style for writing tests, but I have little to no experience there to comment on how effective the error messages are, etc. According to Truth's documents comparing with AssertJ, they suggest that it is another good option with both having their own strengths and weaknesses.

So, there are (up to) two questions open for maintainers:

  1. Should we allow any additional test frameworks?
  2. If so, which test framework(s) should we allow?
@jmrunkle
Copy link
Contributor Author

@jtigger @matthewmorgan @sit @stkent @Smarticles101 @FridaTveit @sjwarner-bp @lemoncurry @sshine @msomji @mirkoperillo @pdmoore

I think that's the only way to get everyone's attention here. If you don't have an opinion, just ignore this.

PS: If you do not want to be a Java maintainer, please LMK and we can remove you from maintainers.json.

@stkent
Copy link
Contributor

stkent commented Apr 16, 2020

Tangent, but: anyone with "alumnus": false in the maintainers.json file is considered an active maintainer; anyone with "alumnus": true is no longer an active maintainer :)

@jmrunkle
Copy link
Contributor Author

Tangent, but: anyone with "alumnus": false in the maintainers.json file is considered an active maintainer; anyone with "alumnus": true is no longer an active maintainer :)

My bad - I'll be more considerate in the future now that I'm aware of the distinction. I always assumed that anyone who was a former maintainer was just removed from the list.

@stkent
Copy link
Contributor

stkent commented Apr 16, 2020

No worries! IIRC the alumnus flag existed so that ex-maintainers could still be credited somewhere on the website (but I don't know if that UI still exists in exercism vLatest). The fun trivia that comes with all long-running projects!

@sshine
Copy link
Contributor

sshine commented Apr 16, 2020

If you do not want to be a Java maintainer, please LMK and we can remove you from maintainers.json.

Possibly tangent, but I was going to say exactly the same:

Don't forget that maintainers.json has an alumnus: true flag. The definition for when someone is an alumnus is when they (or supposedly someone else) pushes that change to maintainers.json. Until then, as I commented in #1785, we don't really ever ask maintainers (or even mentors) to renew their membership.

I want to relate my experience with picking up the SML track: I thought everyone was gone, and suddenly a guy pops up and asks me to justify a commit. :-D I got scared (the wall of text should testify that I got defensive), and one "no sweat!" immediately relieved me.

So as long as we can have that kind of sensitivity when we do step on one another's shoes in all future decisions, I think making bold moves and giving people X weeks or Y days depending on the activity and the matter, the worst-case is that we go back and fix things. Stuff breaks all the time. E.g. #1802 fixes what #733, but #1246 removed it without anybody noticing. No sweat.

the alumnus flag existed so that ex-maintainers could still be credited
I don't know if that UI still exists

It does.

https://exercism.io/team/maintainers
https://exercism.io/team/contributors

@sit
Copy link
Contributor

sit commented Apr 17, 2020

I would support using AssertJ, having used it or its predecessor at the last 3 companies I’ve worked for. Its fluent syntax is easy to read and learn to write, with good IDE support. I don’t have any experience with Truth, perhaps it fits all these criteria as well.

I don’t think there is much burden to the user to add AssertJ to the build (assuming we are still using Gradle or similar build tool that handles dependencies).

I’d also suggest that we standardize on one set of testing libraries across all tests in a track though; although exposure to inconsistency may prepare a student for the real world, having consistency seems like it would be better pedagogically.

@jmrunkle
Copy link
Contributor Author

Seeing as this is a pretty broad change, I want to give everyone at least a week to have a chance to comment.

So far, two out of the six active maintainers support allowing other testing frameworks and would prefer that we use on.

We have two votes for AssertJ and one vote for Truth (maybe one and a half depending on how you interpret sit's last comment).

If you want to keep things easy on yourself, I can make four quick comments that people can thumbs up if they like a particular option (or options). In fact, I'll do that right after this...

@jmrunkle
Copy link
Contributor Author

Like this comment if you want to ban all test frameworks (so no Hamcrest, no Truth, no AssertJ).

@jmrunkle
Copy link
Contributor Author

jmrunkle commented Apr 18, 2020

Like this comment if you want to use Hamcrest (in addition to JUnit).

@jmrunkle
Copy link
Contributor Author

jmrunkle commented Apr 18, 2020

Like this comment if you want to use Truth (in addition to JUnit).

@jmrunkle
Copy link
Contributor Author

jmrunkle commented Apr 18, 2020

Like this comment if you want to use AssertJ (in addition to JUnit).

@pdmoore
Copy link
Contributor

pdmoore commented Apr 18, 2020

If the choice above is JUnit OR AssertJ then I am firmly in the leave it at JUnit camp. If the interest is in exposing them to more frameworks could it be achieved by creating exercises that specifically get them to work with the test framework within that exercise?

I also don't see much interaction during mentoring about tests or the testing framework. Very occasionally there may be mention of missing tests. In my experience the interactions are about learning and practicing the language and not so much about TDD or testing (as much as I value them).

@jmrunkle
Copy link
Contributor Author

If the choice above is JUnit OR AssertJ then I am firmly in the leave it at JUnit camp. If the interest is in exposing them to more frameworks could it be achieved by creating exercises that specifically get them to work with the test framework within that exercise?

This is not about getting rid of JUnit. This is about whether we allow other test framework assertions. We would still rely heavily on JUnit, but we would use the other test framework when it was appropriate (for example, when it gives a better error message). Read the original post up top for arguments for and against allowing other test frameworks.

There is a significant benefit to students that mentors may never see (or hear comments about): if the tests have better assertions because it makes it easier for them to debug their code.

@mirkoperillo
Copy link
Contributor

I agree to use a test library that provides more accurate and easy-to-read assertion messages, it can improve both student experience in error comprehension and maintainers experience to maintain in a easy way the exercises.

I have experience only with Hamcrest, but I don't have any preferences about.
I think the important thing is to stay consistent: choose one framework and document in a clear way what to use in the exercises

@jmrunkle
Copy link
Contributor Author

For comparison, here are the three candidates asserting that a collection is empty and that two objects are equal:

Hamcrest:

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.collection.IsEmptyCollection.empty;
...
assertThat(someCollection, empty());
assertThat(object1, equalTo(object2)); 

Truth:

import static com.google.common.truth.Truth.assertThat;
...
assertThat(someCollection).isEmpty();
assertThat(object1).isEqualTo(object2);

AssertJ:

import static org.assertj.core.api.Assertions.assertThat;
...
assertThat(someCollection).isEmpty();
assertThat(object1).isEqualTo(object2);

As you can see, the APIs for Truth and AssertJ are often identical, and both benefit from IDE auto-completion. Hamcrest also has to import a bunch of extra static matchers.

@jmrunkle
Copy link
Contributor Author

@pdmoore - I'm curious, have you used any of these (Hamcrest, Truth, or AssertJ)? I think that my phrasing of this discussion may have implied that these assertion tools are used outside of JUnit, but they really just work inside JUnit. We are only suggesting switching from JUnit assertions (like assertEquals and assertTrue) to these enhanced assertions where appropriate while still using the JUnit framework for running the tests. I might be overstating things by calling these tools "frameworks" when they are really just enhanced assertions.

Also, this whole question arose because a student was having difficulty troubleshooting a bug in their solution because the test code effectively used assertTrue(someCollection.isEmpty()) which did not give them enough information to debug their code (ie. what is in the collection). See #1634 for the history there.

@pdmoore
Copy link
Contributor

pdmoore commented Apr 20, 2020

@jmrunkle I've used Hamcrest some, and agree it's not as seamless as the other options.

When I read through this thread I got stuck on the suggestion "I’d also suggest that we standardize on one set of testing libraries across all tests in a track though" which implied to me picking one of the three, not adding to JUnit. The voting comments you added didn't express the "JUnit AND this other one" clearly to me. I'm fine with picking one and adding more expressive asserts.

@jmrunkle
Copy link
Contributor Author

We have heard from the majority of the active mentors already and there seems to be reasonable consensus around allowing AssertJ (in addition to JUnit) for assertions. Particularly, in cases where it improves the clarity of the error messaging. Barring additional commentary in the other direction, I will mark the discussion closed tomorrow and create a pull request updating our track policies to say that we allow AssertJ and banning the others.

@msomji and @sjwarner-bp feel free to add your thoughts.

@jmrunkle
Copy link
Contributor Author

Closing, thanks for the input everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants