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

fix(test): Initial test for executing test from mvn in Che workspace terminal shell #463

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

ldimaggi
Copy link
Contributor

@ldimaggi ldimaggi commented Feb 28, 2018

This test verifies the operation of running maven install and test commands within a quickstart/booster's Che workspace terminal window. This test is one of the required tetss defined for boosters.

@ldimaggi ldimaggi force-pushed the booster-terminal-poc-test branch 5 times, most recently from 661f305 to 827c830 Compare February 28, 2018 20:13
@ldimaggi ldimaggi changed the title fix(test): start POC test for booster terminal test - DO NOT MERGE fix(test): Initial test for executing test from mvn in Che workspace terminal shell Feb 28, 2018
await browser.wait(until.textToBePresentInElement(spaceCheWorkSpacePage.bottomPanelTerminal, 'BUILD SUCCESS'), support.LONGER_WAIT);
await expect(spaceCheWorkSpacePage.bottomPanelTerminal.getText()).toContain('BUILD SUCCESS');

await support.printTerminal(spaceCheWorkSpacePage, 'mvn test');

Choose a reason for hiding this comment

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

Why running mvn install and mvn test separated? Inherently install runs both test and integration-test

Also you should consider running mvn clean install -Popenshift,openshift-it to run all the tests Boosters are providing.

block;">...</modal-container>

The only reliable way to avoid this is a sleep statement: await browser.sleep(5000); */
await browser.sleep(5000);

Choose a reason for hiding this comment

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

Making explicit waits for fixed timeout is harmful for two reasons

  • you slow down tests by predefined timeout each and every time, even if the condition is satisfied 10x faster
  • it can be flaky, as it might happen that condition will appear a bit later than the timeout you defined

Is it really the only way to handle this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it turns out - that code is no longer needed - it might have been needed in the past due to the UI being unstable. Either way, the test runs cleanly now without it. Thx!

handles = await browser.getAllWindowHandles();
while (handles.length < 2) {
support.info ('***Waiting for Che browser window to open***');
await browser.sleep (3000);

Choose a reason for hiding this comment

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

Can't we have waits with some pooling and upper limit instead? In the Java world that would be something like:

WebDriverWait wait = new WebDriverWait(driver, 10);
        wait.pollingEvery(200, TimeUnit.MILISECONDS);
        wait.until(ExpectedConditions.elementToBeClickable(button));
        button.click();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx - I found a way around this.

bottomPanelTerminalTab = new Button (element(by.xpath('.//*[contains(@class,\'GDPEHSMCKHC\')][contains(text(),\'Terminal\')]')), 'Che Bottom Panel Terminal Tab');

bottomPanelTerminal = new TextInput (element(by.xpath('.//*[@id=\'gwt-debug-Terminal\']')));
// bottomPanelTerminal = element(by.xpath('(.//*[contains (@class,\'xterm-rows\')]/div)[1]'));

Choose a reason for hiding this comment

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

What's the point of having dead/commented out code in the new commit?

@ldimaggi ldimaggi force-pushed the booster-terminal-poc-test branch from 827c830 to 051cb64 Compare February 28, 2018 20:52
await spaceCheWorkSpacePage.bottomPanelTerminalTab.clickWhenReady();
await support.printTerminal(spaceCheWorkSpacePage, 'cd ' + spaceName);

await support.printTerminal(spaceCheWorkSpacePage, 'mvn clean install -Popenshift,openshift-it');
Copy link

@bartoszmajsak bartoszmajsak Feb 28, 2018

Choose a reason for hiding this comment

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

And this will most likely not work out of the box yet, because we are missing redhat-developer/rh-che#541.

Pretty much what I've described here.

Copy link
Collaborator

@ppitonak ppitonak left a comment

Choose a reason for hiding this comment

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

Test failed for me. I saw this message in Che terminal:

[ERROR] Failed to execute goal io.fabric8:fabric8-maven-plugin:3.5.28:build (fmp) on project testmar11519908074072: Failed to execute the build: Unable to build the image using the OpenShift build service: Failure executing: GET at: https://kubernetes.
default.svc/apis/build.openshift.io/v1/namespaces/ppitonak-che/buildconfigs/testmar11519908074072-s2i. Message: Forbidden!Configured service account doesn't have access. Service account may have been revoked. User "system:serviceaccount:ppitonak-che:de
fault" cannot get buildconfigs.build.openshift.io in the namespace "ppitonak-che": User "system:serviceaccount:ppitonak-che:default" cannot get buildconfigs.build.openshift.io in project "ppitonak-che". -> [Help 1]

await support.printTerminal(spaceCheWorkSpacePage, 'cd ' + spaceName);

await support.printTerminal(spaceCheWorkSpacePage, 'mvn clean install -Popenshift,openshift-it');
await browser.wait(until.textToBePresentInElement(spaceCheWorkSpacePage.bottomPanelTerminal, 'BUILD SUCCESS'), support.LONGER_WAIT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be some logic that will fail the test as soon as 'BUILD FAILURE' appears in console (as happened to me).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent idea!

/* Print text to the Che Terminal window */
// tslint:disable:max-line-length
export async function printTerminal (spaceCheWorkspacePage: SpaceCheWorkspacePage, textToPrint: string) {
await browser.sleep(seconds(3));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of this line?

await browser.driver.actions().mouseDown(spaceCheWorkspacePage.bottomPanelTerminal).click().sendKeys(' ').perform();
await browser.driver.actions().mouseDown(spaceCheWorkspacePage.bottomPanelTerminal).click().sendKeys(textToPrint).perform();
await browser.driver.actions().mouseDown(spaceCheWorkspacePage.bottomPanelTerminal).click().sendKeys(Key.ENTER).perform();
await browser.sleep(seconds(3));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of this line?

let cleanupEnvPage = await editProfilePage.gotoResetEnvironment();
support.debug('>>> Go to Reset Env Page - OK');

await cleanupEnvPage.cleanup(browser.params.login.user);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is too dangerous... it either has to wait for account to be in 'ready' state again of simply assume that the software under test is ready to be used.

Copy link
Contributor Author

@ldimaggi ldimaggi Mar 1, 2018

Choose a reason for hiding this comment

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

This one requires some explanation as it really is a short-term fix to multiple issues.

  • Ideally, every test would reset its environment before running. Resetting the environment enables a test to start with a known (and clean) environment. Also, since our test accounts are all provisioned in the OSO free tier, they are configured with limited resources. Running multiple tests can result in OSO quotas being exceeded.
  • This is not possible at present for multiple reasons - First, resetting the environment is time consuming. Second, the time required is variable. Third, the reset can fail and leave the environment in a state where it must be reset again (perhaps more than once). Fourth and finally, the reset can appear to be successful, but the Jenkins pod is not started.
  • The long term solution is to provide the user with a safe and quick way to "clean up" their OSO resources. This clean up feature does not yet exist. When it is created (after the H train?), the feature to reset the user's environment will be removed or hidden from the user.

Given these^ restrictions, the best short-term approach that I have been able to come up with is this:

  • Run (3) EE tests in sequence - each timed to start on the hour - spaced out over 3 hours. The first 2 tests make use of the build pipeline. The third makes use of Che. The third resets the user env. The wide timing of the tests provides a time buffer for the Jenkins pod to restart before the first test is run again. This is a short-term hack, but it works. We should address this in a better way as soon as we (and the users) have a quick and reliable way to clean up their used OSO resources. Thx!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was not able to successfully run your test and understand why this env reset is present and what problems it has. I would prefer not merging tests that depend on unstable features but I'll leave you to decide if to merge or not.

Choose a reason for hiding this comment

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

Isn't it the only valid reason for tests to exist - to fail? Just my .2$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ldimaggi ldimaggi force-pushed the booster-terminal-poc-test branch 2 times, most recently from ec4eb7f to 241e6c3 Compare March 1, 2018 15:40
@ldimaggi ldimaggi force-pushed the booster-terminal-poc-test branch from 3ae7112 to 8d1cc8a Compare March 2, 2018 23:55
@ldimaggi ldimaggi merged commit f978616 into fabric8io:master Mar 2, 2018
@ldimaggi ldimaggi deleted the booster-terminal-poc-test branch October 19, 2018 17:45
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.

3 participants