Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up[JBIDE-23029] implemented bot test for Scaling with warning #1609
Conversation
This comment has been minimized.
This comment has been minimized.
Can one of the admins verify this patch? |
This comment has been minimized.
This comment has been minimized.
@vpakan I'm looking for feedback so far, please dont merge it yet |
This comment has been minimized.
This comment has been minimized.
@jeffmaury dare to review, too? |
This comment has been minimized.
This comment has been minimized.
@adietish fyi, the message from vpakan is automatic (Vlado is actually no longer in our qa team) and it is entered whenever a user not included in the jenkins job's whitelist creates a PR. |
17fa356
to
d2a8fc6
IProjectRequest request = connection.getResourceFactory().stub(ResourceKind.PROJECT_REQUEST, projectSpec.name()); | ||
request.setDescription(StringUtils.isEmpty(projectSpec.description())? | ||
projectSpec.name() : projectSpec.description()); | ||
request.setDescription(StringUtils.isEmpty(projectSpec.displayName())? |
This comment has been minimized.
This comment has been minimized.
|
||
private static Logger LOGGER = new Logger(OpenShift3ResourceUtils.class); | ||
|
||
public static Connection getConnection(String connectionUrlString) { |
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 14, 2016
Member
Add javadoc, because it is not clear enough without description what form should the URL have. I have to look in other places in code, where its used, that it should look like "https://openshift-dev@10.1.2.2:8443".
* @return | ||
*/ | ||
public static boolean hasProject(String name, Connection connection) { | ||
return getProject(name, connection) != null; |
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 14, 2016
Member
getProject can throw OpenShiftToolsException. Wouldn't it be better also catch such exception and return false in that case? Because if it fails on getting a project, it like it is not there ;)
This comment has been minimized.
This comment has been minimized.
adietish
Sep 21, 2016
Author
Member
I dont see #getProject throwing OpenShiftToolsException. What do I miss?
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 21, 2016
Member
Not directly, but nested. The request in execute method throws the exception "Unable to execute request to...". But maybe it's really rare case and we dont have to bother with it.
@@ -0,0 +1,15 @@ | |||
package org.jboss.tools.openshift.reddeer.requirement; |
This comment has been minimized.
This comment has been minimized.
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.TYPE) | ||
public @interface OpenShift3Project { | ||
String connectionURL(); |
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 15, 2016
Member
Add javadoc for fields in OpenShift3Project annotation so user now how the values should look like. At least for connection which is in a specific form.
This comment has been minimized.
This comment has been minimized.
|
||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.TYPE) | ||
public @interface OpenShift3Service { |
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 15, 2016
Member
Add javadoc for fields in OpenShift3Service annotation so user now how the values should look like. At least for connection which is in a specific form.
ensureServiceExists(serviceSpec.service(), serviceSpec.template(), serviceSpec.project(), connection); | ||
new WaitUntil( | ||
new ServicePodsExist(serviceSpec.service(), serviceSpec.project(), connection) | ||
, TimePeriod.ETERNAL); |
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 15, 2016
Member
It's not a good idea to put ETERNAL waiting. If something goes wrong, all tests would fail on this place and it would crash on timeout instead of continuing with other tests. I would suggest TimePeriod.getCustom() and use a long time out, e.g. 15 minutes should be enough, if not, something is wrong and it fails and its ok.
String password = System.getProperty("openshift.password"); | ||
if (!(password == null || password.equals("") || password.startsWith("${"))) { | ||
String password = System.getProperty(KEY_PASSWORD); | ||
if (!(StringUtils.isEmpty(password) || password.startsWith("${"))) { |
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 15, 2016
Member
Please add to tests/org.jboss.tools.openshift.ui.bot.test/pom.xml empty property for openshift.password and then we can remove following from condition
|| password.startsWith("${")
Also please add to tests/org.jboss.tools.openshift.ui.bot.test/pom.xml to systemProperties openshift.authmethod and also empty property, to make it runnable via mvn from cli.
String token = System.getProperty("openshift.token"); | ||
if (!(token == null || token.equals("") || token.startsWith("${"))) { | ||
String token = System.getProperty(KEY_TOKEN); | ||
if (!(StringUtils.isEmpty(token) || token.startsWith("${"))) { |
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 15, 2016
Member
Can I ask you to also remove || token.startsWith("${")) ? It's not necessary here because of empty property in pom.xml.
This comment has been minimized.
This comment has been minimized.
@adietish I've done review of changes. Please address comments where necessary. |
|
||
String projectName = projectSpec.name(); | ||
Connection connection = OpenShift3ResourceUtils.getConnection(projectSpec.connectionURL()); | ||
if (OpenShift3ResourceUtils.hasProject(projectName, connection)) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 20, 2016
•
Member
if (!connectionDoesNotHaveProject) {
createProject();
waitForProject();
}
but it is just cosmetics
This comment has been minimized.
This comment has been minimized.
test this |
+1, changing as suggested |
This comment has been minimized.
This comment has been minimized.
@rhopp "test this"? |
This comment has been minimized.
This comment has been minimized.
@adietish we use "test this" to trigger verification job |
This comment has been minimized.
This comment has been minimized.
We should probably change the trigger message to something more meaningful like testPR. |
*/ | ||
public class OpenShiftServiceRequirement implements Requirement<RequiredService> { | ||
|
||
private static final TimePeriod TIMEPERIOD_WAIT_FOR_BUILD = TimePeriod.getCustom(30 * 60 * 60); // 30mins |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@adietish please address #1609 (review) and #1609 (review) and rebase PR and then I can do final tests of PR etc. |
2e89046
to
c850c8b
This comment has been minimized.
This comment has been minimized.
@mlabuda addressed your comments and did quite some changes in the code. Before merging, ScalingTest should be removed and PR'd in a a separate PR (it's currently included for testing and design purposes) |
changes requsted in comments are done + further changes in the requirement classes |
IProject project = OpenShiftResourceUtils.getProject(name, connection); | ||
if (project == null) { | ||
LOGGER.debug(NLS.bind("Project {0} doesnt exist yet in {1}, creating it.", name, connection.getHost())); | ||
project = createProject(projectSpec, connection); |
This comment has been minimized.
This comment has been minimized.
mlabuda
Sep 23, 2016
Member
This is not OK. If default values are used ( = usage of requirement without any values in requirement annotation) and project with specified name does not exists, then it in if on line 95 it would continue to condition and on line 97 it fails (com.openshift.restclient.OpenShiftException: Exception trying to POST https://10.1.2.3:8443/oapi/v1/projectrequests response code: 422 ProjectRequest "" is invalid: metadata.name: Required value: name or generateName is required). This is because projectSpec is with no values and in create project its assumed there is value at least for project name
This comment has been minimized.
This comment has been minimized.
adietish
Sep 23, 2016
Author
Member
good catch!
it's using the wrong parameter: it should use the projectName (which is either the one specified in the projectSpec or the default), not the projectSpec which might be missing the project name
406f2c6
to
b7dd2d8
This comment has been minimized.
This comment has been minimized.
UI is not properly updated if test class with requirements is in suite. If I expand a connection in preceding class and than in current test I try to get a project which has been just created by a requirement, it fails because connection tree item is expanded and project is not listed here. Here is minimal classes to reproduce Suite:
First test class:
Second test class:
Assert that connection is clean (there are no projects on a connection) and run test suite with following VM arguments:
it is also reproducible from command line, when running tests via maven, so it definitely need to be fixed |
This comment has been minimized.
This comment has been minimized.
@mlabuda this is very weird indeed. If there's only SecondTest in the suite, everything works fine. If FirstTest and SecondTest are in the suite - while FirstTest is executed first - SecondTest fails where the project is being fetched from explorer. The project actually exists in core but the explorer seems not to get notified of it and therefore doesn't display it. Manually refreshing - while you put a brk-point into SecondTest - fixes the thing. |
This comment has been minimized.
This comment has been minimized.
@mlabuda I found the reason for it. We're currently not watching projects (projects are missing from the list of watched resources in WatchManager). Watching projects is an enhancement that we have filed to https://issues.jboss.org/browse/JBIDE-23184. |
e9a8182
to
2b0fda9
* | ||
* @see WatchManager#KINDS | ||
*/ | ||
new WaitUntil(new OpenShiftProjectExists(name, connection)); |
This comment has been minimized.
This comment has been minimized.
adietish
Oct 10, 2016
Author
Member
this wait (that refreshes the explorer/connection waits for the project to show up) fixes the issue. It should be remove once https://issues.jboss.org/browse/JBIDE-23184 is resolved
This comment has been minimized.
This comment has been minimized.
@mlabuda I rebased and added a workaround/fix that's waiting & refreshing until the project gets visible in UI. Please test again and refute/accept the new PR |
super(ResourceKind.PROJECT, name, null, connection); | ||
} | ||
|
||
// @Override |
This comment has been minimized.
This comment has been minimized.
mlabuda
Oct 11, 2016
Member
Why is this commented out? If it is meant to be used super class test() method, then please remove it, if it is forgotten commented code, please uncomment it.
* @author adietish@redhat.com | ||
* | ||
*/ | ||
public class ResourceExists extends AbstractWaitCondition { |
This comment has been minimized.
This comment has been minimized.
mlabuda
Oct 11, 2016
Member
Would it be possible to merge NamedResourceExists and ResourceExists into one with more constructors and a bit modified logic to satisfy both (connection.getResource(...), if there is a matcher, match response)?
This comment has been minimized.
This comment has been minimized.
adietish
Oct 11, 2016
Author
Member
I dont think that this makes a lot of sense. The little code there is in both and the little code that they can share vs the additionally required checks, switches etc.
return null; | ||
} | ||
|
||
for (IResource resource : connection.getResources(ResourceKind.PROJECT)) { |
This comment has been minimized.
This comment has been minimized.
this(server, username, null); | ||
} | ||
|
||
public AbstractOpenShiftApplicationWizard(String server, String username, String project) { |
This comment has been minimized.
This comment has been minimized.
mlabuda
Oct 11, 2016
Member
Did not you forget to do something with the project parameter? It's completely ignored in constructor body and this constructor is not used anywhere.
*/ | ||
public class NewOpenShift3ApplicationWizard extends NewOpenShiftApplicationWizard { | ||
public class NewOpenShift3ApplicationWizard extends AbstractOpenShiftApplicationWizard { | ||
|
||
public NewOpenShift3ApplicationWizard() { | ||
super(DatastoreOS3.SERVER, DatastoreOS3.USERNAME); |
This comment has been minimized.
This comment has been minimized.
mlabuda
Oct 11, 2016
Member
probably this is the place where constructor with a project parameter should be used and in open methods could be than used the project value?
|
||
replicationController.select(); | ||
scaleTo(0); | ||
Matcher<String> confirmationDialogMatcher = new ShellButtonsMatcher(Arrays.asList("Yes", "No")); |
This comment has been minimized.
This comment has been minimized.
mlabuda
Oct 11, 2016
Member
It's really not necessary to implement such robus matcher and trying to get a shell. Please store shell name in OpenShiftLabel.Shell.STOP_DEPLOYMENT and use here new new DefaultShell(OpenShiftLabel.Shell.STOP_DEPLOYMENT));. Because waiting is default, normal, you don't have to wait for the shell, bcs. shell lookup wait for a period of lenght TimePeriod.NORMAL by default.
This comment has been minimized.
This comment has been minimized.
adietish
Oct 12, 2016
Author
Member
As you said, it's superior, because it does not rely on dialog titles which are fragile. IMHO the solution is not perfect, because it relies on button labels but it is more robust because it does wait for "some dialog which has Yes/No" (to confirm scaling to 0) which is relying on the function of the upcoming dialog more than it specific outfit. Waiting for a dialog with a specific title is less robust. I dont see why we therefore should downgrade, this makes no sense to me (if you have an electric car, would you replace it by a gas driven one?).
This comment has been minimized.
This comment has been minimized.
mlabuda
Oct 12, 2016
•
Member
We have discussed on this topic in QE. We agreed this is not the right way to do stuff. You are not changing shell title. If you want to change shell title, there should be a JIRA to reflect it and not just change. And if there is a JIRA it should be reflected in changes to integration tests. And if precise shell title is not enough, you can still use matcher, which could be a regex matcher matching on keywords. Still far better solution.
Having a shell with Yes/No is less robust because there could pop up some shell which you are not expecting which have Yes/No and it fails there.
replicationController.select(); | ||
scaleTo(0); | ||
Matcher<String> confirmationDialogMatcher = new ShellButtonsMatcher(Arrays.asList("Yes", "No")); | ||
new WaitUntil(new ShellMatchingMatcherIsAvailable(confirmationDialogMatcher)); |
This comment has been minimized.
This comment has been minimized.
mlabuda
Oct 11, 2016
Member
Also, please remove ShellButtonsMatcher and ShellMatchingMatcherIsAvailable. There is a constructor for a DefaultShell with matcher.
cbdcfb0
to
effca30
This comment has been minimized.
This comment has been minimized.
@mlabuda please re-review and free for merge or refute |
This comment has been minimized.
This comment has been minimized.
Please fix ScalingTest issues. Problem is with UI elements. Even requirements create an application, UI is somehow not aware of it and thus it is failing. Basically, there are several issues, one is no existing service, another not running application. |
This comment has been minimized.
This comment has been minimized.
I've tried to pin down the issues. All issues are caused by 2 problems:
|
This comment has been minimized.
This comment has been minimized.
Unfortunately still it is not good. I run ScalingTest 5 times and it failed 5 times on the same exception. There were 2 other projects under the OpenShift connection, there was no test-project at the moment when I started the test. Issue is there was no eap service shown under a project (on screenshot, no item under test-project in OpenShift explorer view, nor any deployments in properties view, although in another IDE which I had opened I am seeing it). And it resulted with this exception: |
This comment has been minimized.
This comment has been minimized.
@mlabuda one more set of changes. Please test them. |
* impl'd requirements for connection, project, service * allow UI assertions to specify connection and project
This comment has been minimized.
This comment has been minimized.
@mlabuda with the latest set of changes I had it green 10/10 times when running ScalingTest solo and starting with non-existing test-project (test requirements create project, service, rc etc.). |
This comment has been minimized.
This comment has been minimized.
@rhopp I'd love if you could give this one a try, it's the required so tha people in openshift can do bot tests efficiently. |
This comment has been minimized.
This comment has been minimized.
I spent quite some time with this PR and I can tell that it runs reliably and I see no obvious thing/problem/error/ with this PR which would prevent it from being merged. |
This comment has been minimized.
This comment has been minimized.
@rhopp great news, thanks a lot for the quick response and intensive verification! |
adietish commentedSep 13, 2016
•
edited
No description provided.