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

Whitespace Control with New Line Trimming Disabled Tests #477

Merged
merged 10 commits into from
Oct 19, 2019
Merged
14 changes: 14 additions & 0 deletions pebble/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,20 @@
</exclusion>
</exclusions>
</dependency>
<!-- https://mvnrepository.com/artifact/org.assertj/assertj-core -->
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

do you really need assertj for your tests ? I'd like to stick with minimal dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ebussieres ebussieres Oct 20, 2019

Choose a reason for hiding this comment

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

<version>3.13.2</version>
<scope>test</scope>
</dependency>
<!-- https://mvnrepository.com/artifact/commons-io/commons-io -->
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.6</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,13 @@ public void testPrintForceToTrue() throws PebbleException, IOException {
assertEquals("val1val2", writer.toString());
}

/**
* Given that Newline Trimming is disabled,
* a template that contains one newline character with text on each line
* should output one newline character.
*/
@Test
public void testPrintSetToFalse() throws PebbleException, IOException {
public void testNewLineIncludedWhen_NewLineTrimmingIsFalse() throws PebbleException, IOException {

PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.newLineTrimming(false)
Expand Down Expand Up @@ -100,8 +105,13 @@ public void testPrintDefaultTwoNewlines() throws PebbleException, IOException {
assertEquals("val1\nval2", writer.toString());
}

/**
* Given that Newline Trimming is disabled,
* a template that contains one or more consecutive newline characters
* should output one newline character.
*/
@Test
public void testPrintSetToFalseTwoNewlines() throws PebbleException, IOException {
public void testOneNewLineWhen_NewLineTrimmingFalseAndConsecutiveNewLinesInTemplate() throws PebbleException, IOException {

PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.newLineTrimming(false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,38 +10,68 @@

import static org.junit.Assert.assertEquals;

import com.mitchellbosecke.pebble.error.PebbleException;
import com.mitchellbosecke.pebble.loader.StringLoader;
import com.mitchellbosecke.pebble.template.PebbleTemplate;
import java.io.IOException;
import java.io.StringWriter;
import java.io.Writer;
import java.util.HashMap;
import java.util.Map;

import org.junit.Test;

import com.mitchellbosecke.pebble.error.PebbleException;
import com.mitchellbosecke.pebble.loader.StringLoader;
import com.mitchellbosecke.pebble.template.PebbleTemplate;

public class WhitespaceControlTest {

/**
* A windows newline character (i.e. \n\r) in a template should be recognized
* and output as as a Windows newline character. The Windows newline character
* should not be converted to a Unix newline character (i.e. \n).
*
* @throws PebbleException
* @throws IOException
*/
@Test
public void testStandardizationOfNewlineCharacters() throws PebbleException, IOException {
public void testWindowsNewlineCharacter() throws PebbleException, IOException {
PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.strictVariables(true).build();

// windows
PebbleTemplate windowsTemplate = pebble.getTemplate("\r\n");
Writer windowsWriter = new StringWriter();
windowsTemplate.evaluate(windowsWriter);
assertEquals("\r\n", windowsWriter.toString());
}

/**
* A Unix newline character (i.e. \n\r) in a template should be recognized
* and output as as a Unix newline character. The Unix newline character
* should not be converted to a Windows newline character (i.e. \r\n).
*
* @throws PebbleException
* @throws IOException
*/
@Test
public void testUnixNewlineCharacter() throws PebbleException, IOException {
PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.strictVariables(true).build();

// unix
PebbleTemplate unixTemplate = pebble.getTemplate("\n");
Writer unixWriter = new StringWriter();
unixTemplate.evaluate(unixWriter);
assertEquals("\n", unixWriter.toString());
}

/**
* A leading Whitespace Control Modifier in an expression delimiter (i.e. Pebble variable reference)
* should remove whitespace before the variable reference on the same line up to any
* surrounding text, i.e. a print delimiter.
*
* @throws PebbleException
* @throws IOException
*/
@Test
public void testLeadingWhitespaceTrimWithPrintDelimiter() throws PebbleException, IOException {
public void testLeadingWhitespaceControlModifier() throws PebbleException, IOException {
PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.strictVariables(true).build();

Expand All @@ -54,36 +84,60 @@ public void testLeadingWhitespaceTrimWithPrintDelimiter() throws PebbleException
assertEquals("<li>bar</li>", writer.toString());
}

/**
* A trailing Whitespace Control Modifier in an expression delimiter (i.e. Pebble variable reference)
* should remove whitespace after the variable reference on the same line up to any
* surrounding text.
*
* @throws PebbleException
* @throws IOException
*/
@Test
public void testLeadingWhitespaceTrimWithoutOutsideText() throws PebbleException, IOException {
public void testTrailingWhitespaceControlModifier() throws PebbleException, IOException {
PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.strictVariables(true).build();

PebbleTemplate template = pebble.getTemplate("{{- foo -}}");
PebbleTemplate template = pebble.getTemplate("<li>{{ foo -}} </li>");
Writer writer = new StringWriter();

Map<String, Object> context = new HashMap<>();
context.put("foo", "bar");
template.evaluate(writer, context);
assertEquals("bar", writer.toString());
assertEquals("<li>bar</li>", writer.toString());
}


/**
* A Whitespace Control Modifier in an expression delimiter (i.e. Pebble variable reference)
* should not have any effect if there is no whitespace immediately before or after the
* variable reference.
*
* @throws PebbleException
* @throws IOException
*/
@Test
public void testTrailingWhitespaceTrimWithPrintDelimiter() throws PebbleException, IOException {
public void testLeadingWhitespaceTrimWithoutOutsideText() throws PebbleException, IOException {
PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.strictVariables(true).build();

PebbleTemplate template = pebble.getTemplate("<li>{{ foo -}} </li>");
PebbleTemplate template = pebble.getTemplate("{{- foo -}}");
Writer writer = new StringWriter();

Map<String, Object> context = new HashMap<>();
context.put("foo", "bar");
template.evaluate(writer, context);
assertEquals("<li>bar</li>", writer.toString());
assertEquals("bar", writer.toString());
}

/**
* A leading and trailing Whitespace Control Modifiers in an expression delimiter
* (i.e. Pebble variable reference) should remove whitespace before and after
* the variable reference on the same line up to any surrounding text.
*
* @throws PebbleException
* @throws IOException
*/
@Test
public void testWhitespaceTrimInPresenceOfMultipleTags() throws PebbleException, IOException {
public void testLeadingAndTrailingWhitespaceControlModifier() throws PebbleException, IOException {
PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.strictVariables(true).build();

Expand All @@ -97,8 +151,15 @@ public void testWhitespaceTrimInPresenceOfMultipleTags() throws PebbleException,
assertEquals("bar <li>bar</li> bar", writer.toString());
}

/**
* Newline characters immediately before or after a Whitespace Control Modifier
* should be removed.
*
* @throws PebbleException
* @throws IOException
*/
@Test
public void testWhitespaceTrimRemovesNewlines() throws PebbleException, IOException {
public void testWhitespaceControlModifierRemovesNewlines() throws PebbleException, IOException {
PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader())
.strictVariables(true).build();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
package com.mitchellbosecke.pebble.template.tests;

import java.io.IOException;
import java.io.StringWriter;
import java.io.Writer;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.HashMap;
import java.util.Map;

import org.apache.commons.io.FileUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.mitchellbosecke.pebble.PebbleEngine;
import com.mitchellbosecke.pebble.loader.StringLoader;
import com.mitchellbosecke.pebble.template.PebbleTemplate;

/**
* Used by Pebble Template Tests to simply the test code and therefore
* make it easier to understand what is tested by each test.
*
* A separate instance of this class should be instantiated for each
* template test, i.e. for each instance of a template.
*
* @author nathanward
*/
public class PebbleTestContext {

private final Logger logger = LoggerFactory.getLogger(PebbleTestContext.class);

/**
* The path relative to the project root directory where template files and
* expected output files are stored for test purposes.
*/
private String testFileBasePath = "src/test/resources/template-tests";

/**
* The Pebble template context to be used as input for a Pebble Template.
*/
private Map<String, Object> templateContext = null;

/**
* Whether or not the Pebble Engine instantiated will
* enable New Line Trimming on the Builder when this class instantiates
* a Pebble Engine instance. Defaults to <code>true</code>.
*/
private boolean newLineTrimming = true;

/**
* Initialize the Pebble Template Context.
*/
public PebbleTestContext() {
this.templateContext = new HashMap<String, Object>();
}

public void setNewLineTrimming(boolean b) {
this.newLineTrimming = b;
}

/**
* Put an object into the Pebble template context to be used as input for
* when the template is executed. One or more items can be put into the template
* context.
*
* @param name Template input/parameter name (i.e. key) for use within the template
* @param value The object that will be referred to by the given name in the template
*/
public void setTemplateInput(String name, Object value) {
this.templateContext.put(name, value);
}

/**
* Load the specified template file and execute the template using a
* Pebble Engine using the default Builder (classpath and file builder).
*
* @param templateFilename The template filename relative to the Test File Base Path
*
* @return The output of the template as a string.
*
* @throws IOException Thrown if the template file is not found.
*/
public String executeTemplateFromFile(String templateFilename) throws IOException {
PebbleEngine pebbleEngine = new PebbleEngine.Builder()
.newLineTrimming(this.newLineTrimming).strictVariables(true).build();
return this.executeTemplateFromFile(templateFilename, pebbleEngine);
}

/**
* Execute a template given a template file and a Pebble Engine instance.
*
* @param templateFilename The template filename relative to the Test File Base Path
* @param pebbleEngine
* @return
* @throws IOException
*/
public String executeTemplateFromFile(String templateFilename, PebbleEngine pebbleEngine) throws IOException {
Path path = Paths.get(this.testFileBasePath, templateFilename);
logger.debug("Executing template file: {}", path.toString());
return this.executeTemplate(path.toAbsolutePath().toString(), pebbleEngine);
}

/**
* Load the specified template file and execute the template using a
* Pebble Engine using the default Builder (classpath and file builder).
*
* @param templateString The template content as a string
*
* @return The output of the template as a string.
*
* @throws IOException Thrown if the template file is not found.
*/
public String executeTemplateFromString(String templateString) throws IOException {
PebbleEngine pebbleEngine = new PebbleEngine.Builder().loader(new StringLoader())
.newLineTrimming(this.newLineTrimming).build();
return this.executeTemplate(templateString, pebbleEngine);
}

/**
* Load the specified template file and execute the template using the template input
* that has previously been specified using the setTemplateInput() method.
*
* @param templateFilename The template filename relative to the Test File Base Path
* @param pebbleEngine The Pebble Engine to be used to execute the template
* @return The output of the template as a string.
*
* @throws IOException Thrown if the template file is not found.
*/
public String executeTemplate(String templateName, PebbleEngine pebbleEngine) throws IOException {
PebbleTemplate template = pebbleEngine.getTemplate(templateName);
Writer writer = new StringWriter();
template.evaluate(writer, this.templateContext);
String templateOutput = writer.toString();
logger.debug("Template Output: {}", templateOutput);
return templateOutput;
}

/**
* Get the Expected Output content for the given filename so that the
* base path to the expected template output file does not have to be
* specified in the actual test code.
*
* @param filename The name of the file that contains the expected template output.
* @return The content of the expected template output file as a string.
* @throws IOException Thrown if the file by the given name is not found.
*/
public String getExpectedOutput(String filename) throws IOException {
Path path = Paths.get(this.testFileBasePath, filename);
logger.debug("Expected template output file: {}", path.toAbsolutePath());
String expectedOutput = FileUtils.readFileToString(path.toFile(), "UTF-8");
return expectedOutput;
}

}
Loading