added support for execution order for "Before" and "After" hook. closes #807 #809
Conversation
| this.tagExpression = tagExpression; | ||
| this.timeoutMillis = timeoutMillis; | ||
| this.timeout = timeout; |
There was a problem hiding this comment.
Can you switch it back to timeoutMillis please? I've been bitten too many times by bugs caused by wrong assumptions about units.
|
LGTM when my minor comments are addressed. |
| for (Object o : tagsExpressionsAndBody) { | ||
| if (o instanceof String) { | ||
| tagExpressions.add((String) o); | ||
| } else if (o instanceof Integer) { | ||
| timeoutMillis = (Integer) o; | ||
| } else if (o instanceof Number && isDefaultTimeout) { |
There was a problem hiding this comment.
I think if we used instanceof Long and instanceof Integer we could use that to detect timeout and order, respectively. In other words - Long means timeout - Integer means order.
There was a problem hiding this comment.
I would assume groovy developers using numbers without caring about the type (long or int). so we will end up codes like Before(1, 100, "tag1", "tag2") {}.
Don't you think it would be more error prone?
There was a problem hiding this comment.
Does Groovy have a literal syntax for longs like Java does? I.e. 500L.
We could throw an error if you pass two or more ints and longs I suppose. There is still a danger that people would pass in 2000 hoping that would set the timeout, when in fact it would set the order (since 2000 is an int). But you have the same problem with your current implementation.
Alternatively we could break backwards compatibility and expect a Map:
Before([order: 5, timeoutMillis: 4000L, tags:["@tag1", "@tag2"]]) {
}I don't mind too much breaking backwards compatibility if it is for a good reason.
There was a problem hiding this comment.
in the current implementation, we assume that the first number is always the timeout and only if the second number is available in the parameters it will be considered as order. This was mainly because I was thinking that there are existing code out there that passing the timeout, so their behaviour should remain the same.
If you or ok with breaking backward compatibility, we can have proper methods (overloaded) with following syntax:
public static void Before(int order, long timeoutMillis, List<String> tags, Closure body)
//default order
public static void Before(long timeoutMillis, List<String> tags, Closure body)
//default timeout
public static void Before(int order, List<String> tags, Closure body)
// default tags
public static void Before(int order, long timeoutMillis, Closure body)
and ...
or even better if we implement it in groovy, we could have it with one method by default values.
static void Before(int order = 0, long timeoutMillis = 10000, List<String> tags = [], Closure body)
There was a problem hiding this comment.
forgot to answer your first question.
Yes groovy does have those suffixes. http://groovy.codehaus.org/Groovy+Math
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Since the original method was accepting varargs (
Object... args) as parameter, there was almost no way to add new method with proper parameters, without breaking backward compatibility.