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.

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

JBIDE-25314 CDK launch configuration itests #1609

Merged
merged 1 commit into from
Nov 27, 2017

Conversation

odockal
Copy link
Member

@odockal odockal commented Nov 6, 2017

- New test class CDKLaunchConfigurationTest
- CDKSmoke and CDKAllTests suites extended with new test class
- CDKLaunchConfigurationDialog added into cdk.reddeer plugin
- New mock files representing cdk binaries
- Remove deprecated CDK3 test classes
- Add check of OS and docker after restart

Signed-off-by: Ondrej Dockal odockal@redhat.com

Pull Request Checklist

General

  • Is this a blocking issue or new feature? If yes, QE needs to +1 this PR

Code

  • Are method-/class-/variable-names meaningful?
  • Are methods concise, not too long?
  • Are catch blocks catching precise Exceptions only (no catch all)?

Testing

  • Are there unit-tests?
  • Are there integration tests (or at least a jira to tackle these)?
  • Is the non-happy path working, too?
  • Are other parts that use the same component still working fine?

Function

  • Does it work?

@odockal odockal changed the title CDK launch configuration itests JBIDE-25314 - CDK launch configuration itests Nov 6, 2017
@odockal odockal changed the title JBIDE-25314 - CDK launch configuration itests JBIDE-25314 CDK launch configuration itests Nov 6, 2017
@odockal
Copy link
Member Author

odockal commented Nov 9, 2017

testPR

1 similar comment
@adietish
Copy link
Member

testPR

@adietish
Copy link
Member

adietish commented Nov 13, 2017

@odockal i must be missing something: When launching the tests I see it complaining about a profile related error

Failed tests:
  CDK32ServerWizardTest.testNewCDK32ServerWizard:85->CDKServerWizardAbstractTest.assertSameMessage:144 Expected page description should contain text: is not compatible with this server adapter but has: A server adapter representing Red Hat Container Development Kit Version 3.2+
  CDK32ServerEditorTest.testInvalidMinishiftLocation:125->CDKServerEditorAbstractTest.checkEditorStateAfterSave:120->CDKServerEditorAbstractTest.verifyEditorCannotSave:178 Editor was saved successfully but exception was expected
  CDKLaunchConfigurationTest.testMinishiftHomePropagationIntoLaunchConfig:163 expected:<.../adietish/.minishift[Folder]> but was:<.../adietish/.minishift[]>
  CDK32ServerAdapterStartTest.testStartServerAdapter:44->CDKServerAdapterAbstractTest.startServerAdapter:135 Server's state went back to STOPPED
<terminated> Container Development Environment 3.2 [Red Hat CDK Server Startup Configuration] /Applications/cdk3/cdk-320
open /Users/adietish/.minishift/machines/minishift/config.json: no such file or directory

  CDK32ServerAdapterRestartTest.testCDERestart:46->CDKServerAdapterAbstractTest.startServerAdapter:135 Server's state went back to STOPPED
<terminated> Container Development Environment 3.2 [Red Hat CDK Server Startup Configuration] /Applications/cdk3/cdk-320
open /Users/adietish/.minishift/machines/minishift/config.json: no such file or directory

  CDK32ServerAdapterConnectionTest.testCDK3ServerAdapterConnection:43->CDKServerAdapterAbstractTest.startServerAdapter:135 Server's state went back to STOPPED
<terminated> Container Development Environment 3.2 [Red Hat CDK Server Startup Configuration] /Applications/cdk3/cdk-320
open /Users/adietish/.minishift/machines/minishift/config.json: no such file or directory


Tests run: 13, Failures: 6, Errors: 0, Skipped: 0

image

@adietish
Copy link
Member

@robstryker dare to test this please?

@odockal
Copy link
Member Author

odockal commented Nov 14, 2017

@adietish The error in the pic suggests that there is something wrong with .minishift folder, perhaps try to delete old minishift config (minishift delete, remove .minishift and .kube...) and then minishift setup-cdk from fresh. I have never seen such error before.

@jkopriva
Copy link
Member

jkopriva commented Nov 14, 2017

@adietish for variables -Dminishift= and -Dminishift.profile= you must enter absolute path, otherwise the test will be failing, also for CDK32 tests you also need to set up -Dminishift= variable(something like: -Dminishift=/home/jkopriva/git/jkopriva/jbosstools-openshift/itests/org.jboss.tools.cdk.ui.bot.test/resources/cdk-files/linux/cdk-3.1.1-mock), @odockal is working on better variables handling

otherwise tests are working for me without errors

@robstryker
Copy link
Member

When I run the tests, I get tons of failures.

Tests run: 28, Failures: 12, Errors: 3, Skipped: 0

[rob@rawbdorable org.jboss.tools.cdk.ui.bot.test] (cdk-launch)$ mvn clean install -DskipTests=false -DskipITests=false -PITests -Dminishift.hypervisor=virtualbox -Dminishift=/home/rob/Downloads/cdk/minishift/cdk3.1.1.20170911/minishift -Dminishift.profile=/home/rob/Downloads/cdk/minishift/cdk32a1/minishift -Ddevelopers.username=rstryker@redhat.com -Ddevelopers.password=xxxxx -Dsecurestorage.password=xxxxx -Dvagrantfile=/home/rob/Downloads/cdk/cdk-install/25-Jan-2017.rc0/cdk/components/rhel/rhel-ose/ -Dusage_reporting_enabled=false

One of the problems (for me) seems to be that the @RemoveCDKServers requirement doesn't seem to be running. If I change a number of dialog.finish(TimePeriod.MEDIUM); with dialog.cancel();
in the following classes, I get much closer to a green build (and sometimes 100% success):

CDK32ServerWizardTest.java
CDK3ServerWizardTest.java
CDKServerWizardTest.java

I've run these tests repeatedly for a day now and I can't really allocate any more time towards them. I also believe this patch needs to be rebased against master, because JBIDE-25308 will cause some of the tests to fail, and this was only committed recently.

@adietish
Copy link
Member

adietish commented Nov 15, 2017

@odockal re-installing cdk helped. But using CDK 3.2-rc2 and running the tests in the following way:

mvn clean verify -Dminishift.hypervisor=xhyve -Dminishift=/Applications/cdk3/cdk-320 -Dminishift.profile=/Applications/cdk3/cdk-320 -Ddevelopers.username=adietish@redhat.com -Ddevelopers.password=XXXXX -Dusage_reporting_enabled=false -PITests -Pcdk32-all-tests

I unfortunately now face the following errors (which match the errors that @robstryker reported above):

Failed tests:
  CDK32ServerWizardTest.testNewCDK32ServerWizard:85->CDKServerWizardAbstractTest.assertSameMessage:144 Expected page description should contain text: is not compatible with this server adapter but has: A server adapter representing Red Hat Container Development Kit Version 3.2+
  CDK32ServerEditorTest.testInvalidMinishiftLocation:125->CDKServerEditorAbstractTest.checkEditorStateAfterSave:120->CDKServerEditorAbstractTest.verifyEditorCannotSave:178 Editor was saved successfully but exception was expected
  CDKLaunchConfigurationTest.testMinishiftHomePropagationIntoLaunchConfig:163 expected:<.../adietish/.minishift[Folder]> but was:<.../adietish/.minishift[]>

Tests run: 13, Failures: 3, Errors: 0, Skipped: 0

org jboss tools cdk ui bot test server editor cdk32servereditortest testinvalidminishiftlocation
org jboss tools cdk ui bot test server editor launch cdklaunchconfigurationtest testminishifthomepropagationintolaunchconfig
org jboss tools cdk ui bot test server wizard cdk32serverwizardtest testnewcdk32serverwizard

@adietish
Copy link
Member

correcting the cmd-line to the following:

mvn clean verify -Dminishift.hypervisor=xhyve -Dminishift=/Applications/cdk3/cdk-311 -Dminishift.profile=/Applications/cdk3/cdk-320 -Ddevelopers.username=adietish@redhat.com -Ddevelopers.password=XXXXX -Dusage_reporting_enabled=false -PITests -Pcdk32-all-tests

I now have 1 error left:

Failed tests:
  CDKLaunchConfigurationTest.testMinishiftHomePropagationIntoLaunchConfig:163 expected:<.../adietish/.minishift[Folder]> but was:<.../adietish/.minishift[]>

image

@odockal
Copy link
Member Author

odockal commented Nov 15, 2017

@robstryker @adietish I thank you guys for the feedback! Today I have planned to incorporate your comments into code.

  1. First thing,
Failed tests:
  CDKLaunchConfigurationTest.testMinishiftHomePropagationIntoLaunchConfig:163 expected:<.../adietish/.minishift[Folder]> but was:<.../adietish/.minishift[]>

this failure should be solved as #1606 was merged recently, so uage of newest devstudio should do the job. But I must say that I wrote the test before the PR was merged and thus I did not test it myself that it is working, will check today.

  1. @robstryker @removecdkservers requirement is truly not running properly as there is a bug causing that requirement is not run in in @after part of test if test failed. (Requirement.runAfter is not called when test fails eclipse/reddeer#1847) I will fix this today as well. (not the bug in reddeer but the cleaning in after part).

  2. JBIDE-25308 have to check this. (including rebasing).

  3. @robstryker Thing with dialog.finish(timeout) vs. dialog.cancel(). This is a huge pain. I would consider clicking finish button as a compulsory part of successful test case (wizard test, editor/adapter test), wouldn't it? Of course, It might be omitted in wizard testing as the same thing is being tested in two other test cases (editor/adapter), as you suggested, I will try this.
    To make it clear why this is happening: Problem here is that finish button pressing triggers
    "Refreshing server adapter list" job, this is one of the most favorite jobs to handle, it is not triggered always, takes non constant time to finish and is hard to kill :D Code waits for one minute giving the job time to finish itself, then it tries to kill the job, sometimes it does not work.

Thanks for patience with itests!

@odockal odockal force-pushed the cdk-launch branch 2 times, most recently from ff36763 to c1f05eb Compare November 15, 2017 21:31
@Override
public void runAfter() {
deleteCDKServers();
// log.info("Deleting all CDK server adapters in runAfter");

Choose a reason for hiding this comment

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

MAJOR This block of commented-out lines of code should be removed. rule

@rhdevelopers-ci
Copy link

SonarQube analysis reported 1 issue

  • MAJOR 1 major

Watch the comments in this conversation to review them.

@adietish
Copy link
Member

Running the latest version of this PR I unfortunately still get the following error:

Results :

Failed tests:
  CDKLaunchConfigurationTest.testMinishiftHomePropagationIntoLaunchConfig:163 expected:<.../adietish/.minishift[Folder]> but was:<.../adietish/.minishift[]>

Tests run: 13, Failures: 1, Errors: 0, Skipped: 0

image

@odockal
Copy link
Member Author

odockal commented Nov 17, 2017

Do you have some newer devstudio or jbt? It is running for me with devstudio version 11.2.0-v20171115-0753-B1626.

@adietish
Copy link
Member

i am running from master branch aka source plugins.


private CDKLaunchConfigurationDialog launchDialog;

private static final Logger log = Logger.getLogger(CDKLaunchConfigurationTest.class);
Copy link
Member

Choose a reason for hiding this comment

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

good practice for static variables is to name them in capital letters. Preferrable would be for instance

private static final Logger LOG

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that in this case we should always use lower case as we use "log" as a final reference instance of Logger class. It is always used to access Logger's method such as log or debug, so always in format log.info(...) or log.error(...). I like this convention more as you can type without using shift. And of course I found something on stackoverflow (https://stackoverflow.com/questions/1417190/should-a-static-final-logger-be-declared-in-upper-case) which support my individual preference. After all, all above is my feeling and I believe that usage of log in lower case is the most used convention across itests repos. but int the end, this is decision of contributors in given plugin. I would vote for using lower case for final reference variable that are used to access methods (thus always followed with a dot, please, see https://web.archive.org/web/20120911192801/developers.sun.com/sunstudio/products/archive/whitepapers/java-style.pdf - Field naming).

@adietish
Copy link
Member

adietish commented Nov 27, 2017

running the latest PR (via maven) I have

Failed tests:
  CDKLaunchConfigurationTest.testMinishiftHomePropagationIntoLaunchConfig:163 expected:<.../adietish/.minishift[Folder]> but was:<.../adietish/.minishift[]>

Tests run: 13, Failures: 1, Errors: 0, Skipped: 0

Weird enough, when running CDKLaunchConfigurationTest individually in the IDE everything is green, there are no failures.
Same is true if I launch CDK32AllTestsSuite in Eclipse. All Tests pass, it's green.

@adietish
Copy link
Member

@robstryker dare to run this and confirm/refute please?

@adietish
Copy link
Member

i have this finally running fine, the problem being outdated maven artifacts. Killing .m2/org.jboss.tools.openshift and reinstalling them helped.

Results :

Tests run: 13, Failures: 0, Errors: 0, Skipped: 0

adietish
adietish previously approved these changes Nov 27, 2017
	- New test class CDKLaunchConfigurationTest
	- CDKSmoke, CDK32AllTests and CDKAllTests suites extended with new test class
	- CDKLaunchConfigurationDialog added into cdk.reddeer plugin
	- New mock files representing cdk binaries
	- Remove deprecated CDK3 test classes
	- Add check of OS and docker after restart

Signed-off-by: Ondrej Dockal <odockal@redhat.com>
@adietish
Copy link
Member

with latest master installed as maven artifacts, running this fails for me on

Results :

Failed tests:
  CDKLaunchConfigurationTest.testServerNamePropagationIntoLaunchConfig:126 expected:<...ment Environment 3.2[x]> but was:<...ment Environment 3.2[]>

Tests run: 13, Failures: 1, Errors: 0, Skipped: 0

Copy link
Member

@adietish adietish left a comment

Choose a reason for hiding this comment

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

runs fine in Eclipse. Merging.

@adietish adietish merged commit e03b2a8 into jbosstools:master Nov 27, 2017
@odockal odockal deleted the cdk-launch branch June 25, 2018 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants