-
Notifications
You must be signed in to change notification settings - Fork 49
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
Rerun one test / Rerun suite #337
Conversation
@@ -49,6 +49,9 @@ public void setStepResult(ComparatorStepResult stepResult) { | |||
} | |||
|
|||
public List<Operation> getFilters() { | |||
if(filters == null){ | |||
return new ArrayList<>(); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be a little bit confusing why I compare filters with null, filters have initial value new ArrayList<>
so they never are null
, but if the array is empty, in the database we haven't got information about it, this field doesn't exist. It was the easiest way to solve NullPointerException
, what do you think about saving filters:[]
in the database when the array is empty? Or maybe you know another way to solve it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is managed by Suite.GSON_FOR_JSON
Gson instance. It is used in toJson
method. It has registered several adapters, one of them is to manage java Collection
conversions. You may see the logic of dealing with null
collections here:
https://github.com/Cognifide/aet/blob/master/api/communication-api/src/main/java/com/cognifide/aet/communication/api/metadata/gson/CollectionSerializer.java#L30
Let's just be sure that changing logic here won't break metadata documents saved before this change is introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can leave it as is, without refactoring GSON_FOR_JSON
logic. But it should return Collections.emptyList()
instead of new ArrayList<>()
.
}, | ||
getDomain: function () { | ||
var config = { | ||
'getUrl': domain | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is a result of change suitestatus
to api/suitestatus
. After that servlet returns api/..
, and after concatenation I will have /api/api/suitestatus
-> incorrect URL. I can escape api
from one of this URL, but I guess it isn't good practice. In additional I didn't want to manipulate URL returned by the servlet, it could be problematic for the client and create more problems for this easy case so I decided to add the new function with a used domain from a variable. Is it a good way? Will it require from users some changes in their configs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at my review comment. This can't be programmed like now.
public class ProgressLog { | ||
import java.io.Serializable; | ||
|
||
public class ProgressLog implements Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add serialVersionUID
to this and all Serializable
classes.
@@ -49,6 +49,9 @@ public void setStepResult(ComparatorStepResult stepResult) { | |||
} | |||
|
|||
public List<Operation> getFilters() { | |||
if(filters == null){ | |||
return new ArrayList<>(); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can leave it as is, without refactoring GSON_FOR_JSON
logic. But it should return Collections.emptyList()
instead of new ArrayList<>()
.
import java.util.List; | ||
import java.util.Optional; | ||
|
||
public abstract class RunIndexWrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
com.cognifide.aet.communication.api.wrappers.Run
is a parameterized class. Shouldn't template information be also kept in this class?
I mean
public abstract class RunIndexWrapper<T> {
protected Run<T> objectToRunWrapper = null;
...
urlRunWrapper.getProxy(), urlRunWrapper.getPreferredBrowserId()); | ||
ObjectMessage message = session.createObjectMessage(data); | ||
message.setJMSCorrelationID(correlationId); | ||
messagesQueue.add(new MessageWithDestination(getQueueOut(), message, 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please create an issue with technical debt
label about refactoring MessageWithDestination
to hold always only one message?
|
||
###### Request | ||
|
||
* **URL**: `/suite-rerun` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is /api/suite-rerun
now.
* **URL**: `/suite-rerun` | ||
* **HTTP Method**: POST | ||
* **Parameters**: | ||
* `correlationId` - Correlation ID of the suite to rerun, used to get it from the database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to also support company, project, suite, version
?
Does that make sense?
It could be done as an improvement in separate PR. If you think this is valid, please create proper ticket.
@@ -18,15 +18,21 @@ | |||
define(['angularAMD'], function (angularAMD) { | |||
'use strict'; | |||
angularAMD.factory('endpointConfiguration', function () { | |||
var domain = 'http://aet-vagrant'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't be hardcoded! :)
Otherwise it won't work on different environments than http://aet-vagrant
.
This is why getUrl
returns relative url /api
.
Please don't change this. Your domain is exactly the domain, at which this report is displayed.
Look how it is done in metadataEndpoint
to call the endpoint without domain provided.
}, | ||
getDomain: function () { | ||
var config = { | ||
'getUrl': domain | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at my review comment. This can't be programmed like now.
var rerunParams = 'company=' + suiteInfo.company + '&' + 'project=' + suiteInfo.project + '&' + | ||
'suite=' + suiteInfo.name + '&' + 'testName=' + testName; | ||
var url = endpointConfiguration.getEndpoint().getUrl + 'suite-rerun?' + rerunParams; | ||
console.log(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove console.log
, this shouldn't be used in production ready code in that form.
@@ -82,7 +82,7 @@ void unregister(String servletPath) { | |||
setHttpService(null); | |||
} | |||
|
|||
boolean isValidName(String suiteName) { | |||
public static boolean isValidName(String suiteName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If those methods are used outside the servlet scope please extract them.
BasicDataServlet
purpose shouldn't be mixed with Service/Helper that provide some common functionalities.
…e-way-to-execute-suite
We added ability to rerun one test or whole suite
Description
We created new endpoint
/rerun-suite
which get information about company, project and correlationId. With these parameters whole suite will be started. If you also provide testName, aet will create fake suite with one test and override result in database. Rerun option is avaible only for the latest suite.Screenshots (if appropriate):
Types of changes
Checklist:
I hereby agree to the terms of the AET Contributor License Agreement.