Whitespace Control with New Line Trimming Disabled Tests #477
Conversation
…ng is enabled. Also added assertj and commons-io Maven dependencies for use in the tests. Commons IO is used to compare template output to expected output that is loaded from a file. Loading the expected template output from a file is useful for testing multiline template output, which is necessary to test various scenarios. These are a form of Integration Test rather than Unit Tests and so it is practical and reasonable to verify the template output in this way.
…hanged the test method names in some cases to make the purpose of each test method more clear.
…atement Thatis Skipped test
|
@nward1234 build is failing |
|
I looked at the build failure details. It does not show anything that I can
tell would be specific to the code or the changes that I made. Is it
possible that TravisCI could not connect to get some sort of build
resources at the time? The build passed for Java 8 and failed for Java 11
and 12. I had been using Java 8, but I installed Java 11 after seeing the
build failure. The clean build ran and tests passed locally with Java 11
for me. I will be glad to look into it further, but I am at a loss to
determine the root cause so far.
Nathan
…On Fri, Sep 27, 2019 at 10:58 AM Eric Bussieres ***@***.***> wrote:
@nward1234 <https://github.com/nward1234> build is failing
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#477?email_source=notifications&email_token=AA6EQGXBJPWSVNVXXU6R7YDQLYNQXA5CNFSM4IZJ7XLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7ZFN3Q#issuecomment-535975662>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA6EQGRQGRNENNJP6U22W2TQLYNQXANCNFSM4IZJ7XLA>
.
|
|
I'll cancel the pull request and submit a new one and see if the same error
occurs again. If it does, then it must be something that I did to cause it.
If no errors, then it was a temporary TravisCI build issue.
Nathan
On Fri, Sep 27, 2019 at 8:14 PM Nathan Ward <nathanward1234@gmail.com>
wrote:
… I looked at the build failure details. It does not show anything that I
can tell would be specific to the code or the changes that I made. Is it
possible that TravisCI could not connect to get some sort of build
resources at the time? The build passed for Java 8 and failed for Java 11
and 12. I had been using Java 8, but I installed Java 11 after seeing the
build failure. The clean build ran and tests passed locally with Java 11
for me. I will be glad to look into it further, but I am at a loss to
determine the root cause so far.
Nathan
On Fri, Sep 27, 2019 at 10:58 AM Eric Bussieres ***@***.***>
wrote:
> @nward1234 <https://github.com/nward1234> build is failing
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#477?email_source=notifications&email_token=AA6EQGXBJPWSVNVXXU6R7YDQLYNQXA5CNFSM4IZJ7XLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7ZFN3Q#issuecomment-535975662>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA6EQGRQGRNENNJP6U22W2TQLYNQXANCNFSM4IZJ7XLA>
> .
>
|
|
Cool! Closed and reopened so that Travis build would kick off again and the build passed. |
| <!-- https://mvnrepository.com/artifact/org.assertj/assertj-core --> | ||
| <dependency> | ||
| <groupId>org.assertj</groupId> | ||
| <artifactId>assertj-core</artifactId> |
There was a problem hiding this comment.
do you really need assertj for your tests ? I'd like to stick with minimal dependencies
There was a problem hiding this comment.
Since I see that we have Hamcrest matchers, I would not mind using the Junit org.junit.Assert.assertTha() with Hamcrest Matchers instead of AssertJ. However, I would rather use AssertJ because it is more readable than Hamcrest, although either is much more readable than JUnit alone. AssertJ also is easier to use than Hamcrest because the fluent methods allows IDEs to provide method autosuggestions whereas you have to know the Matcher class names and include static imports for each Hamcrest matcher.
Would you say more about the reason for sticking with minimal dependencies? I'd like to understand your concerns.
There was a problem hiding this comment.
Eric. I see that you merged the pull request. Did you decide that you are OK with the additional AssertJ dependency? If not, let me know and I will change it. I would like to hear more about why minimal dependencies is important to you. I plan to make more tests and hope to continue contributing to Pebble. So, I would like to understand your concerns and I want to be sure that you are fine with my changes.
There was a problem hiding this comment.
Yeah i'm ok with assertJ. I personnaly use it too. I just don't want to have a lot of unit tests made with differents libraries (some with google truth, others with assertj or junit or hamcrest, etc)
There was a problem hiding this comment.
Ah! Now, that makes sense. I certainly can understand and agree with that. I think that we should have some standards like this. We can always decide to change the standard and then migrate to the new standard overtime when there is a reason to do so. At some point soon, I would be glad to replace the small amount of Hamcrest Matchers used in the code and standardize on AssertJ. I would not mind also using AssertJ where JUnit eventually, but I would think that would be a lower priority. I would also be glad to start documenting coding standards as needed. It would be just documenting existing standards that aren't down on paper yet so people know up front. In any case, I'll continue using AssertJ and get started on the larger task of addressing the whitespace control issues.
There was a problem hiding this comment.
It appears that your standards indentation is 2 characters in Java code. I can live with that if that is your standard. However, I'd like to confirm that you prefer 2 characters. I'm used to 4 characters for Java, which is the default for Eclipse. I know that Javascript is typically 2 characters, but I have never seen 2 characters used for Java before. So, I just wanted to confirm that you want it to be 2 characters. If so, I will continue with 2 characters, which is what I have done in my changes so far to be consistent with the existing code.
There was a problem hiding this comment.
Yes I use google style as coding standard. It is available here : https://github.com/PebbleTemplates/pebble/blob/master/intellij-java-google-style.xml
or https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml
I added tests for whitespace control for templates when New Line Trimming is disabled. Also added assertj and commons-io Maven dependencies for use in the tests. Commons IO is used to compare template output to expected output that is loaded from a file. Loading the expected template output from a file is useful for testing multiline template output, which is necessary to test various scenarios. These are a form of Integration Test rather than Unit Tests and so it is practical and reasonable to verify the template output in this way.
Related issue: #470 (comment)