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.

Sign up for GitHub

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

Jump to bottom

Implemented partial parameter definitions #525

Closed
wants to merge 1 commit into from
Closed

Implemented partial parameter definitions #525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 97 additions & 24 deletions spock-core/src/main/java/org/spockframework/compiler/WhereBlockRewriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,27 @@

package org.spockframework.compiler;

import java.util.*;

import org.codehaus.groovy.ast.*;
Copy link
Member

Choose a reason for hiding this comment

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

please don't change the ordering of the imports and also avoid whitespace changes, where nothing really happens, this makes this commit just harder to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The imports are done by Idea automatically.

The end of line trimming is an .editorconfig setting, provided with the project: trim_trailing_whitespace = true

Would it be possible to configure these setting the way you would like to receive them, and reformat the whole codebase with it (and commit the config also).

It shouldn't take more than an hour, and would save all these reviews about formatting.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately .editorconfig is too limited to really cover the whole spock formatting and not all code really has the correct one, however if you are using IntelliJ you can configure the formatting to only apply to your changed code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would gladly unify the styles and provide a config for Idea and .editorconfig and reformat the whole codebase to avoid this confusion (so that new people have the correct settings from the start - it's not ok that everyone has to configure the styles on their own, and it's also not ok that the code is not uniform, and it's also not ok that I have to manually revert the imports and the trailing spaces every time I modify something)

import org.codehaus.groovy.ast.expr.*;
import org.codehaus.groovy.ast.stmt.*;
import org.codehaus.groovy.ast.stmt.BlockStatement;
import org.codehaus.groovy.ast.stmt.ExpressionStatement;
import org.codehaus.groovy.ast.stmt.ReturnStatement;
import org.codehaus.groovy.ast.stmt.Statement;
import org.codehaus.groovy.control.SourceUnit;
import org.codehaus.groovy.syntax.Token;
import org.codehaus.groovy.syntax.Types;
import org.objectweb.asm.Opcodes;

import org.spockframework.compiler.model.WhereBlock;
import org.spockframework.runtime.model.DataProviderMetadata;
import org.spockframework.util.*;
import org.spockframework.util.InternalIdentifiers;
import org.spockframework.util.Nullable;
import org.spockframework.util.ObjectUtil;

import java.util.*;

import static org.spockframework.compiler.AstUtil.createGetAtMethod;
import static org.spockframework.util.ObjectUtil.firstNonNull;
import static org.spockframework.util.ReflectionUtil.getDefaultValue;

/**
*
Expand Down Expand Up @@ -68,11 +74,37 @@ private void rewrite() {
resources.getErrorReporter().error(e);
}

validateParameterOrder();

whereBlock.getAst().clear();
handleFeatureParameters();
addFeatureParameters();
createDataProcessorMethod();
}

private void validateParameterOrder() {
Iterator<VariableExpression> dataIterator = dataProcessorVars.iterator();

for (Parameter parameter : whereBlock.getParent().getAst().getParameters()) {
String parameterName = parameter.getName();

if ((find(dataIterator, parameterName) == null) && isDataProcessorVariable(parameterName)) {
String message = String.format("Parameter '%s' is in the wrong order!", parameterName);
resources.getErrorReporter().error(new InvalidSpecCompileException(parameter, message));
return;
}
}
}

@Nullable
private VariableExpression find(Iterator<VariableExpression> dataIterator, String name) {
while (dataIterator.hasNext()) {
VariableExpression variable = dataIterator.next();
if (name.equals(variable.getText()))
return variable;
}
return null;
}

private void rewriteWhereStat(ListIterator<Statement> stats) throws InvalidSpecCompileException {
Statement stat = stats.next();
BinaryExpression binExpr = AstUtil.getExpression(stat, BinaryExpression.class);
Expand All @@ -87,15 +119,14 @@ private void rewriteWhereStat(ListIterator<Statement> stats) throws InvalidSpecC
rewriteSimpleParameterization(binExpr, stat);
else if (leftExpr instanceof ListExpression)
rewriteMultiParameterization(binExpr, stat);
else
else
notAParameterization(stat);
} else if (type == Types.ASSIGN)
rewriteDerivedParameterization(binExpr, stat);
else if (getOrExpression(binExpr) != null) {
stats.previous();
rewriteTableLikeParameterization(stats);
}
else
} else
notAParameterization(stat);
}

Expand Down Expand Up @@ -273,19 +304,19 @@ private void splitRow(Expression row, List<Expression> parts) {
splitRow(orExpr.getRightExpression(), parts);
}
}

private BinaryExpression getOrExpression(Statement stat) {
Expression expr = AstUtil.getExpression(stat, Expression.class);
return getOrExpression(expr);
}

private BinaryExpression getOrExpression(Expression expr) {
BinaryExpression binExpr = ObjectUtil.asInstance(expr, BinaryExpression.class);
if (binExpr == null) return null;

int binExprType = binExpr.getOperation().getType();
if (binExprType == Types.BITWISE_OR || binExprType == Types.LOGICAL_OR) return binExpr;

return null;
}

Expand Down Expand Up @@ -314,12 +345,6 @@ private void verifyDataProcessorVariable(VariableExpression varExpr) {
resources.getErrorReporter().error(varExpr, "Duplicate declaration of data variable '%s'", varExpr.getName());
return;
}

if (whereBlock.getParent().getAst().getParameters().length > 0 && !(accessedVar instanceof Parameter)) {
resources.getErrorReporter().error(varExpr,
"Data variable '%s' needs to be declared as method parameter",
varExpr.getName());
}
}

private boolean isDataProcessorVariable(String name) {
Expand All @@ -344,16 +369,64 @@ private void checkAllParametersAreDataVariables(Parameter[] parameters) {
}

private void addFeatureParameters() {
Parameter[] parameters = new Parameter[dataProcessorVars.size()];
for (int i = 0; i < dataProcessorVars.size(); i++)
parameters[i] = new Parameter(ClassHelper.DYNAMIC_TYPE, dataProcessorVars.get(i).getName());
whereBlock.getParent().getAst().setParameters(parameters);
MethodNode methodNode = whereBlock.getParent().getAst();

List<Parameter> newParameters = new ArrayList<Parameter>();

Map<String, Parameter> parameterTypes = getParameters(methodNode);
newParameters.addAll(getDataProcessorParameters(parameterTypes));
newParameters.addAll(getExtraParameters(parameterTypes));

methodNode.setParameters(newParameters.toArray(new Parameter[newParameters.size()]));
}

private Map<String, Parameter> getParameters(MethodNode methodNode) {
Map<String, Parameter> result = new LinkedHashMap<String, Parameter>();
for (Parameter parameter : methodNode.getParameters())
result.put(parameter.getName(), parameter);
return result;
}

private List<Parameter> getDataProcessorParameters(Map<String, Parameter> parameterTypes) {
List<Parameter> results = new ArrayList<Parameter>();
for (VariableExpression variableExpression : dataProcessorVars) {
String name = variableExpression.getName();
Parameter parameter = firstNonNull(parameterTypes.remove(name),
new Parameter(ClassHelper.DYNAMIC_TYPE, name));
validateDefaultValue(parameter);
results.add(parameter);
}
return results;
}

private List<Parameter> getExtraParameters(Map<String, Parameter> parameterTypes) {
List<Parameter> results = new ArrayList<Parameter>();
for (Parameter parameter : parameterTypes.values()) {
validateDefaultValue(parameter);
ClassNode type = getNullableType(parameter);
results.add(new Parameter(type, parameter.getName()));
}
return results;
}

private void validateDefaultValue(Parameter parameter) {
if (parameter.hasInitialExpression()) {
String message = String.format("Parameter '%s' has default value, which is not supported yet!", parameter.getName());
resources.getErrorReporter().error(new InvalidSpecCompileException(parameter, message));
}
}

private ClassNode getNullableType(Parameter parameter) {
ClassNode type = parameter.getType();
if (type.getTypeClass().isPrimitive())
type = ClassHelper.make(getDefaultValue(type.getTypeClass()).getClass());
return type;
}

@SuppressWarnings("unchecked")
private void createDataProcessorMethod() {
if (dataProcessorVars.isEmpty()) return;

dataProcessorStats.add(
new ReturnStatement(
new ArrayExpression(
Expand Down
27 changes: 20 additions & 7 deletions spock-core/src/main/java/org/spockframework/runtime/ParameterizedSpecRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@

package org.spockframework.runtime;

import org.spockframework.runtime.model.*;

import java.util.*;

import static java.util.Arrays.asList;
import static org.spockframework.runtime.RunStatus.*;
import org.spockframework.runtime.model.*;

/**
* Adds the ability to run parameterized features.
Expand Down Expand Up @@ -55,8 +57,8 @@ private Object[] createDataProviders() {
MethodInfo method = dataProviderInfo.getDataProviderMethod();
Object[] arguments = Arrays.copyOf(dataProviders, getDataTableOffset(dataProviderInfo));
Object provider = invokeRaw(sharedInstance, method, arguments);
if (runStatus != OK)

if (runStatus != OK)
return null;
else if (provider == null) {
SpockExecutionException error = new SpockExecutionException("Data provider is null!");
Expand Down Expand Up @@ -136,14 +138,25 @@ private void runIterations(Iterator[] iterators, int estimatedNumIterations) {
if (runStatus != OK) return;

while (haveNext(iterators)) {
initializeAndRunIteration(nextArgs(iterators), estimatedNumIterations);
initializeAndRunIteration(getInitializedDataValues(iterators), estimatedNumIterations);

if (resetStatus(ITERATION) != OK) break;
// no iterators => no data providers => only derived parameterizations => limit to one iteration
if(iterators.length == 0) break;
}
}

/* TODO we should take the default parameter values into consideration, instead of initing them always with null? */
private Object[] getInitializedDataValues(Iterator[] iterators) {
Collection<Object> dataValues = new ArrayList<Object>(asList(nextArgs(iterators)));

int extraParameterCount = currentFeature.getDataVariables().size() - dataValues.size();
for (int i = 0; i < extraParameterCount; i++)
dataValues.add(null);

return dataValues.toArray();
}

private void closeDataProviders(Object[] dataProviders) {
if (action(runStatus) == ABORT) return;
if (dataProviders == null) return; // there was an error creating the providers
Expand Down Expand Up @@ -193,7 +206,7 @@ private SpockExecutionException createDifferentNumberOfDataValuesException(DataP

// advances iterators and computes args
private Object[] nextArgs(Iterator[] iterators) {
if (runStatus != OK) return null;
if (runStatus != OK) return EMPTY_ARGS;

Object[] next = new Object[iterators.length];
for (int i = 0; i < iterators.length; i++)
Expand All @@ -202,15 +215,15 @@ private Object[] nextArgs(Iterator[] iterators) {
} catch (Throwable t) {
runStatus = supervisor.error(
new ErrorInfo(currentFeature.getDataProviders().get(i).getDataProviderMethod(), t));
return null;
return EMPTY_ARGS;
}

try {
return (Object[])invokeRaw(sharedInstance, currentFeature.getDataProcessorMethod(), next);
} catch (Throwable t) {
runStatus = supervisor.error(
new ErrorInfo(currentFeature.getDataProcessorMethod(), t));
return null;
return EMPTY_ARGS;
}
}
}
10 changes: 9 additions & 1 deletion spock-core/src/main/java/org/spockframework/util/ObjectUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ public static boolean eitherNull(Object... objs) {
return false;
}

public static <T> T firstNonNull(T... objs) {
for (T obj : objs) {
if (obj != null)
return obj;
}
throw new IllegalArgumentException("All elements were null!");
}

@SuppressWarnings("unchecked")
public static @Nullable <T> T asInstance(Object obj, Class<T> type) {
return type.isInstance(obj) ? (T) obj : null;
Expand All @@ -57,4 +65,4 @@ public static <T extends Comparable<T>> int compare(T comparable1, T comparable2
if (comparable2 == null) return 1;
return comparable1.compareTo(comparable2);
}
}
}
Loading