OTT BE Testing #1
Conversation
|
All service impl classes can be auto-generated, I will do it for you when I have the time. In a second thought, you better remove all these classes and implement one executeSync method in TestAPIOkRequestsExecutor and use it directly. |
|
Try to delete any created object after the test is done, with success or error, that will enable us to run these tests also in production without trashing the DB too much. |
|
All service speific utils could be implemented in a parent class of all tests of that service, would be much more elegant. |
| @@ -0,0 +1,100 @@ | |||
| package com.kaltura.client.test; | |||
There was a problem hiding this comment.
We don't want these properties to be hard-coded, please load them from properties file and create both template file similar to https://github.com/kaltura/KalturaOttGeneratedAPIClientsJava/blob/master/src/test/java/resources/ott.test.template.properties and real configuration for QA env.
There was a problem hiding this comment.
@tan-tan-kanarek What do you mean "both template file"? we already have test.properties file but it is git ignored because of sensitive data.
There was a problem hiding this comment.
If you have properties file, why I see all the values here hard-coded?
Also, you can commit the properties file that contain all the QA inaccessible URLs.
Third, please supply a template file for the properties file with place holders for all values.
| private static ILogger logger = Logger.getLogger(TAG); | ||
| protected static TestAPIOkRequestsExecutor self; | ||
|
|
||
| public static TestAPIOkRequestsExecutor getExecutor() { |
There was a problem hiding this comment.
Please check coding standards for singletons: https://www.journaldev.com/1377/java-singleton-design-pattern-best-practices-examples
There was a problem hiding this comment.
|
|
||
| @SuppressWarnings("rawtypes") | ||
| @Override | ||
| protected ResponseElement onGotResponse(Response response, RequestElement action) { |
There was a problem hiding this comment.
Reimplementing onGotResponse will cause that you won't get any change that done on the parent class that is the one that used in real life.
I suggest to call the super.onGotResponse to get the response object and then assert it in case of erroneous response.
There was a problem hiding this comment.
| // set configuration | ||
| config = new Configuration(); | ||
| config.setEndpoint(API_BASE_URL + "/" + API_URL_VERSION); | ||
| config.setAcceptGzipEncoding(false); |
There was a problem hiding this comment.
I believe that at least some tests should run with gzip enabled, both for JSON and XML and other serve actions in other response formats, it should be tested as well.
|
|
||
| public static Client getClient(String ks) { | ||
| Client client = new Client(config); | ||
| client.setApiVersion(API_REQUEST_VERSION); |
There was a problem hiding this comment.
This should never be set, the hard-coded generated API version should be used.
There was a problem hiding this comment.
@tan-tan-kanarek Not sure what do you mean? can you please elaborate?
There was a problem hiding this comment.
The API version is already part of the generated client and contains the exact version of the server that this client was generated from, if you change it to older version you miss all the new features that this lib offers, if you change it to higher version some features in the lib won't be supported by the server.
You better not touch it.
| Client client = getClient(null); | ||
|
|
||
| if (sharedHousehold == null) { | ||
| sharedHousehold = createHouseHold(2, 2, true); |
There was a problem hiding this comment.
Please try to avoid hard-coded numbers.
| return sharedHousehold; | ||
| } | ||
|
|
||
| public static String getsharedMasterUserKs() { |
There was a problem hiding this comment.
Please note to coding standard, the S must be capitalized.
Also, would be nice to have a comment that explain the meaning of shared user vs. non-shared one.
There was a problem hiding this comment.
Fixed. Regarding the shared parameters they are used as global resources which needed in multiple scenarios. Also:
// shared ks's
private static String administratorKs, operatorKs, managerKs, anonymousKs;
// shared VOD
private static MediaAsset mediaAsset;
There was a problem hiding this comment.
Please add a comment in the code when and what they're used for.
| public class Sandbox extends BaseTest { | ||
|
|
||
| @Test(enabled = false) | ||
| private void test() { |
There was a problem hiding this comment.
Inline explanation should describe the purpose of this class and its methods, it's not clear when or why it's used.
There was a problem hiding this comment.
This is just sandbox place for debug purposes. No functionality. I can add it to git ignore if you would like.
There was a problem hiding this comment.
If it serves the tests, keep it in git and add comment that explains it, otherwise, remove it.
|
|
||
| import static com.kaltura.client.test.Properties.*; | ||
|
|
||
| public class DBUtils extends BaseUtils { |
There was a problem hiding this comment.
I believe that this class may grow into gigantic size with time, please consider to split it to class per object type, e.g. EpgChannelDbUtils.
# Conflicts: # src/main/java/com/kaltura/client/Client.java
added more tests for inheritance checking
# Conflicts: # src/test/java/com/kaltura/client/test/tests/featuresTests/versions/five_zero_two/IngestVodOpcTests.java
# Conflicts: # src/main/java/com/kaltura/client/Client.java
@tan-tan-kanarek
"All service impl classes can be auto-generated, I will do it for you when I have the time."
That will be great as it will save us a lot of maintenance time. Also wanted to ask you if in each impl function we need AtomicBoolean for parallel execution?
"Try to delete any created object after the test is done, with success or error, that will enable us to run these tests also in production without trashing the DB too much."
Will do, but only in case there's API for it. (I've been asked not to delete directly from the DB).
"All service specific utils could be implemented in a parent class of all tests of that service, would be much more elegant."
If you like I can do so, but we calling those utils from different services tests. For example createHousehold() being called almost in each test (not only in HouseholdTests). Let me know what you think.