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

Use concrete assertions, not exists #23271

Merged

Conversation

stacey-gammon
Copy link
Contributor

A few places to switch over from exists to existsOrFail and missingOrFail.

@stacey-gammon stacey-gammon added Feature:Dashboard Dashboard related features flaky-failing-test Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 18, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

Looks like a flaky failure:
API Integration Tests.test/api_integration/apis/saved_objects/migrations·js.apis saved_objects Kibana index migration Coordinates migrations across the Kibana cluster

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented Sep 18, 2018 via email

@stacey-gammon
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

Of course.

Failure: Browser Unit Tests.kibana_map tests.kibana_map tests KibanaMap - baseLayer TMS

jenkins, test this

@LeeDr
Copy link

LeeDr commented Sep 20, 2018

I keep having failures on test
dashboard app using legacy data dashboard save Saves on confirm duplicate title warning
which might be unrelated to this PR. But I thought I'd note what I found here.

What I see when the test is running is that the confirm save dialog is open with a message that a dashboard with the name already exists. The test is supposed to click the Confirm Save button, and it does, but nothing happens.

I added this debug logging in dashboard_page.js;

    async clickSave() {
      await retry.try(async () => {
        log.debug('clicking final Save button for named dashboard');
        const button = await testSubjects.find('confirmSaveDashboardButton');
        log.debug(`------------- button = ${await button.getProperty('innerHTML')}`);

        return await testSubjects.click('confirmSaveDashboardButton');
      });
    }

And I see some debug logs like;
------------- button = <span class="euiButton__content"><span class="euiButton__text">Confirm Save</span></span>

but the one right at this point of failure is;
------------- button = <span class="euiButton__content"><div class="euiLoadingSpinner euiLoadingSpinner--medium euiButton__spinner"></div><span class="euiButton__text">Confirm Save</span></span>

So I think that clickSave method needs to check that there isn't any
class="euiLoadingSpinner euiLoadingSpinner--medium euiButton__spinner"

This seems to fix it;

    async clickSave() {
      await retry.try(async () => {
        log.debug('clicking final Save button for named dashboard');
        await retry.waitFor('confirm save to be ready', async () => {
          const button = await testSubjects.find('confirmSaveDashboardButton');
          const innerHtml = await button.getProperty('innerHTML');
          log.debug(`------------- button = ${innerHtml}, Spinner? = ${innerHtml.includes('Spinner')}`);
          return !innerHtml.includes('Spinner');
        });
        return await testSubjects.click('confirmSaveDashboardButton');
      });
    }

A more elegant solution might be to add a data-test-subj to that spinner div and wait for that to be gone? But that might require deciding on a timeout. There's no timeout on getting the innerHTML of the button that we already found. Just a timeout on the try.waitFor.

We shouldn't need the outer retry in clickSave if we wait for the spinner to be gone. It's not solving the current problem.

Or, maybe the button should be disabled and the testSubjects.click would check that and wait for it to be enabled.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon
Copy link
Contributor Author

@LeeDr - can you confirm whether the test failures only show up for you with this PR, and not with master as well? If it's directly related to this PR, then that's one thing, but if it exists in master right now, I'd prefer to get this in and focus on the other issue separately.

So I think that clickSave method needs to check that there isn't any
class="euiLoadingSpinner euiLoadingSpinner--medium euiButton__spinner"

Yes, I know, this is the issue. This was fixed in another PR that implemented a clickWhenNotDisabled function. This can be tackled externally to this PR though, as long as my changes aren't making the situation any worse than it already is now.

I commented here: #22409 (comment) about the issue, thought I commented in slack too but maybe that message got lost. We could perhaps file a more targeted issue for it. I was hoping just to get a fix in for it but ended up taking much longer than expected, hence the stalled PR here: #22901

Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - the other issue can be fixed in another PR. I ran this locally, reviewed code, and checked Jenkins results.

@stacey-gammon stacey-gammon merged commit 8065308 into elastic:master Sep 21, 2018
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Sep 21, 2018
@liza-mae liza-mae added failed-test A test failure on a tracked branch, potentially flaky-test and removed flaky-failing-test labels Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
failed-test A test failure on a tracked branch, potentially flaky-test Feature:Dashboard Dashboard related features Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants