opencharles / charles-rest

Github chatbot and web-content indexer/searcher
http://charles.amihaiemil.com
BSD 3-Clause "New" or "Revised" License
24 stars 10 forks source link

Let's start using hamcrest matchers #178

Open amihaiemil opened 7 years ago

amihaiemil commented 7 years ago

Currently all the tests are done using the simple asserts (mostly assertTrue and assertFalse) from class org.junit.Assert. This gets the job done, it's the standard way of doing assertions in junit.

However, hamcrest matchers are:

for instance, when a simple assert breaks, you'll only see a raw AssertionException (e.g. here), whereas an error from hamcrest will clearly say something like "...expected x, but got y..". This behaviour could also be achieved with simple asserts, but it's more laborious, you have to supply the error messages for every assert.

Convert simple assertions to hamcrest assertions for 2 or 3 test classes and leave a puzzle in the code so the job gets continued on a future task.

I would start with the easy ones, like EnglishTestCase, ErrorReplyTestCase and LogsOnServerTestCase.

See this test class for examples on how to use hamcrest matchers.

Any questions, feel free to ask here.

amihaiemil commented 7 years ago

@dwkielman I think tasks regarding unit tests are good to start with because you get to know the classes involved and how they are used. So I give this to you :)

I'll elaborate the description soon enough. Of course, you won't have to change all the test classes - just a few (how many you can do in 30 min) and leave a puzzle in the code for the rest. I'll explain as soon as I get some time

dwkielman commented 7 years ago

This sounds perfect, I'll get to this today/tonight and if I have any issues getting it running will let you know.

amihaiemil commented 7 years ago

@dwkielman wait for my details and remember, don't lose sleep ^^

Also, you wont have to get the app running (as in, on a jee server) for this task. Just make sure the maven project opens properly in the IDE and you can run the unit tests from there.

amihaiemil commented 7 years ago

@dwkielman I updated the description with all the details. Don't forget to fork the repo, clone it and do the changes on a separate branch named 178 (master is always read-only).

When you're done, make a PR and we'll discuss any code review changes there.

Keep in mind that you don't need to setup the whole project running on a server (some time will pass until you'll have to do that, and ideally, when you'll do it, you'll know the project architecture at a medium level at least).

For this task, just make sure the project builds fine and you can run the unit tests from the IDE.

dwkielman commented 7 years ago

@amihaiemil I got everything set up on my end and with a separate branch, needed to do some git tutorial but I think I have everything working on my end now. If you have a specific test you want me to work on you can assign it to me and I'm happy to start working on any without losing sleep :)

amihaiemil commented 7 years ago

@dwkielman well, this is the task and it's assigned to you. Pick 2 or 3 test classes which you can do in 30 min (I suggested some in the description) and add a puzzle in the code for the rest - I'll explain the puzzle format when you make a PR.

And as I said, if you have any questions, ask here :P

dwkielman commented 7 years ago

@amihaiemil gotcha, I didn't know if there was a specific test you wanted me to work on but I'll find one that sounds good and follow the format and give it a go.

Any issues I'll bring up here :)

dwkielman commented 7 years ago

@amihaiemil Forgive my early stages of programming, but I really am learning a lot here. Got the project in my IDE (using IntelliJ, used Gradle before not too familiar with maven but figuring it out) and got the hamcrest dependency added to the xml. Before I get I was gonna start with EnglishTestCase and convert the test cases already there into hamcrest format, correct? Wanted to be sure before doing too much and getting here is a learning process for me, but thank you for your patience and help! :)

amihaiemil commented 7 years ago

@dwkielman Don't apologize, we've all been there :P

Yes, EnglishTestCase is good to start with. All you have to do is convert the asserts into matchers hamcrest asserts (check the description, in the last line I put a link to a test class that uses them, to have as example). No logic of the test needs to change.

I cannot tell you how different is Gradle from Maven because I've never used Gradle. For starters, all your interaction with Maven will be running the build command mvn clean install and making sure it passes. If it fails, you'll see in the logs the reason (usually a failed unit test) and will be able to fix it.

When a task will come that will require deeper knowledge of it, I'll clearly explain. RIght now, you added that dependency in pom.xml in order for Maven to fetch it from the central repository and use it at testing phase (notice the <scope>test</scope>) - this means that the library won't be in the final build, since it's not needed at the runtime of the application, only at compile time and test runs.

I would advise you not to struggle to integrate Maven with the IDE (if it's not out of the box). It's easiest if you simply run the command from the terminal (in the project's root folder, where pom.xml is).

Note that you might have problems with Maven if you run behind a firewall or stuff like that, it might not be able to fetch its dependencies. Let me know if you run into stuff like this.

dwkielman commented 7 years ago

Thanks @amihaiemil for all the advice!

I tried running that build command mvn clean install but not sure if I was in the right folder. Maven was easy to integrate, it's an option to select when creating the project so wasn't too tough to get that going, I more just have to familiarize myself with the way a Maven build works and I'm all for learning that too. Doesn't seem too difficult :)

I'll check that the build passes and if it doesn't see what I can find out before asking for help again and will start on the unit tests. Thanks again for all the resources for helping with this too and explaining the differences, didn't know about this type of error checking so it's cool to see what else is out there.

amihaiemil commented 7 years ago

@dwkielman If Maven is integrated in your IDE, then you should be able to run the build from there (somewhere it should ask you to type in the "goals", which are clean install).

If you can't figure that out, simply go in the project's directory, open a terminal and run mvn clean install:P

Also, intelliJ is quite popular, so any google search about maven and intellij should get your answer in the first result :D

dwkielman commented 7 years ago

@amihaiemil Thanks for pointing that out. Yes, those were definitely all terms I saw, "goals" and whatnot, I just had no idea what they did haha. And yup, I've been able to find some great tutorials on getting it running with intellij so hasn't been difficult to find support, I just want to make sure I'm getting it right too since it's my first time in this direction but it'll be cake in no time :)

amihaiemil commented 7 years ago

@dwkielman One more point: there is Travis CI integrated in all the repos and also rultor is used when merging, so basically any build failure will be caught and I'll explain if you can't figure it out :)

dwkielman commented 7 years ago

@amihaiemil Two issues I'm encountering so far. Tried this for over an hour and not sure if one is involving the other or what's happening and realized I'm staring at my screen too long.

I got the mvn clean install to work but I'm getting an error. Here's what it looks like: `Results :

Failed tests: structuresPagesCorrectly(com.amihaiemil.charles.aws.EsBulkJsonTestCase): The 2 structures are not the same! (did you forget to add a final newline (\n)?

Tests run: 108, Failures: 1, Errors: 0, Skipped: 0

[INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ [INFO] Total time: 21.147s [INFO] Finished at: Thu Jan 12 21:30:52 PST 2017 [INFO] Final Memory: 21M/57M [INFO] ------------------------------------------------------------------------ [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.10:test (default-test) on project charles-rest: There are test failures. [ERROR] [ERROR] Please refer to C:\Users\Ryan\Desktop\charles-rest\target\surefire-reports for the individual test results. [ERROR] -> [Help 1] [ERROR] [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch. [ERROR] Re-run Maven using the -X switch to enable full debug logging. [ERROR] [ERROR] For more information about the errors and possible solutions, please read the following articles: [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

Process finished with exit code 1`

If you need the rest of it I can post that somewhere.

Regardless, tried working on one of the test cases to convert the EnglishTestCase to hamstring and reading up on it but struggling with the program's function for generating the Language or Command objects. I get what I need to make it do (I think) but I think I'm misunderstanding some way to check that the language is English. Doing one of the tests in the code already, I was trying to take something like :

Command hello1 = this.mockCommand("@charlesmike, hello there!"); Language english = new English();

I can see that an assertThat of some type will be used after this but can't seem to get the method to do it correctly and I think it involves my understanding of how these are generated.

Sorry for the amateur question, that's where I'm at, open to hearing what I need to understand to get this going. Thanks for the examples and help so far and patience :)

amihaiemil commented 7 years ago

@dwkielman i'll answer in about 1-2h, when I get to a laptop :)

dwkielman commented 7 years ago

@amihaiemil Thanks! I'll be asleep but will work on it in the morning :)

amihaiemil commented 7 years ago

@dwkielman sure, no worries

amihaiemil commented 7 years ago

@dwkielman To answer the first question:

Failed tests: structuresPagesCorrectly

Answer The problem is that, right now, the file src/test/resources/bulkIndexStructure.txt has Windows-style line endings, on your machine. That's why the test fails.

It will pass once you convert the line endings to Unix-like endings. As I see at a first google search, this is not so easy to do in IntelliJ (it's one click in Eclipse, hehe), but you can do it in Notepad++ (see here)

Reason Windows and UNIX (Linux, ubuntu, MacOS etc) operating systems have different ways of dealing with line-endings (Windows uses CRLF, while UNIX uses LF).

When you installed git on your machine, you probably had to choose between 3 options of dealing with line endings, and you chose the first one (if not, it did this by default). See here.

This shouldn't be a problem anymore now, but if you want to be sure you're never going to have problems with line endings again, it's best I think if you set the second option (check-out as is, commit as it) - because IDEs can deal with both line endings. To do that, run the command:

git config --global core.autocrlf input, as explained in that SO thread.

Finally All of this shouldn't have been a problem, but I forgot to declare .txt and .json files in the .gitattributes file. One of this .gitattributes file's purposes is to fix this line-endings problem globally, for everyone. So as you're probably guessing, it's a good practice that every git repository has such a file.

As you see in it, all the matching files are automatically checked-out in UNIX style, no matter what settings you have on your machine.

I fixed it now, I declared those file patterns so even if you don't change your local settings, you shouldn't have problems anymore (with this repo!). You should, still, change the line endings of the .txt file in order to make the test run.

Please, let me know if this is clear now and if not, ask any questions you might have :D

amihaiemil commented 7 years ago

@dwkielman To answer your second question:

struggling with the program's function for generating the Language or Command objects

Why are you struggling with that? :D You don't have to write any new tests, or alter any code in the existing ones, except for the asserts

So you look at a test's code, the asserts are at the end: you literally don't have to touch any line above the first assert, but simply translate those asserts into matchers asserts. Example of matcher asserts you can find in this test class.

Here's EnglishTestCase.recognizesHelloCommands() done, as example:

    /**
     * A 'hello' command is understood.
     */
    @Test
    public void recognizesHelloCommands() throws Exception {
        Command hello1 = this.mockCommand("@charlesmike, hello there!");
        Command hello2 = this.mockCommand("@charlesmike, hello?!");
        Command hello3 = this.mockCommand("@charlesmike hi");
        Command hello4 = this.mockCommand("@charlesmike, how do you do??");
        Command hello5 = this.mockCommand("@charlesmike who are you?");

        Language english = new English();
        MatcherAssert.assertThat(
            english.categorize(hello1).type(), Matchers.equalTo("hello")
        );
        MatcherAssert.assertThat(
            english.categorize(hello2).type(), Matchers.equalTo("hello")
        );
        MatcherAssert.assertThat(
            english.categorize(hello3).type(), Matchers.equalTo("hello")
        );
        MatcherAssert.assertThat(
            english.categorize(hello4).type(), Matchers.equalTo("hello")
        );
        MatcherAssert.assertThat(
            english.categorize(hello5).type(), Matchers.equalTo("hello")
        );
    }

^ Notice the 4-space indentation, which is like that because lines were longer than 80 chars :)

General format of a matcher assert is:

MatcherAssert.assertThat(tested_object, Matcher)

In our case, tested_object is a string (the command type returned by method .type()) and the Matcher used is the one for String equality (most of the times, you will get any Matcher you need by calling one of the static methods of class org.hamcrest.Matchers).

As you're probably guessing, the most commonly used Matcher is the one for equality, but there are also matchers for null checks, empty interables checks, contains etc etc. You'll catch it on the fly for sure.

Also, google is filled with resources, since hamcrest matchers is quite commont. Just google something like "how to check for empty list with hamcrest matchers" for example.

Any other questions you might have, please ask :) And make a PR after you do EnglishTestCase class - I think you spent more than 30 min anyway, so that should be enough :D