fixing flaky management settings test

[bmcconaghy]

Closes #20893

I was able to reproduce the original failure (commented out all the other tests, ran the settings ones in a loop). After this fix I was not able to reproduce it again.

[cjcenizal]

Code LGTM! Let's run the CI a few times.

So what was the original problem here? Was it that setFindTimeout returns a promise and we were calling findByCssSelector on it? Or that the await applies to the promise of the last method in the chain, i.e. click?

[cjcenizal]

Retest

[cjcenizal]

Retest

[cjcenizal]

Retest

[cjcenizal]

Looks like Jenkins doesn't like duplicate builds running. 🤕

[bmcconaghy]

I'm actually not sure exactly what was wrong with the original. It looked fishy to me though, so I tried rewriting it in the idiom that I see more often in the code.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[bmcconaghy]

jenkins test this

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[jen-huang]

Code LGTM, but I'm also suspicious of what the root cause is. We have another method in this same file called toggleAdvancedSettingCheckbox() that is essentially the same pattern, but using testSubjects.find instead of .findByCssSelector. I'm concerned that if this is a flaky pattern, we'll run into flakiness in a lot of other areas.

[stacey-gammon]

I know this is already merged but I think the best way to write this (for future reference) is

instead of this:

      let option;
      await retry.try(async () => {
        option = await remote.findByCssSelector(
          `[data-test-subj="advancedSetting-editField-${propertyName}"] option[value="${propertyValue}"]`
        );
      });
      await option.click();

Do this:

await find.clickByCssSelector(`[data-test-subj="advancedSetting-editField-${propertyName}"] option[value="${propertyValue}"]`);

Not only does that give us the retry loop, because there is a retry loop in there:

    async clickByCssSelector(selector, timeout = defaultFindTimeout) {
      log.debug(`clickByCssSelector(${selector})`);
      await retry.try(async () => {
        const element = await this.byCssSelector(selector, timeout);
        await remote.moveMouseTo(element);
        await element.click();
      });
    }
  }

but it also takes advantage of find.byCssSelector which ensures the element can be clicked:

 async byCssSelector(selector, timeout = defaultFindTimeout) {
      log.debug(`findByCssSelector ${selector}`);
      return await this._ensureElementWithTimeout(timeout, async remote => {
        return await remote.findByCssSelector(selector);
      });
    }

Specifically that _ensureElementWithTimeout option does some nice stabilizing stuff.

[stacey-gammon]

@jen-huang, that part of toggleAdvancedSettingCheckbox looks okay to me, but it would be better to use

testSubjects.click(advancedSetting-editField-${propertyName}); in one go, since it has the outer retry loop, and also adds the moveMouseTo in case the elment is off screen or something.

[bmcconaghy]

Thanks @stacey-gammon for the knowledge. I am happy to rewrite it in that fashion. Should we try somehow to make that the canonical way to do things? Seems like part of the problem with these tests is that there are so many ways to do the same thing, with some having hidden disadvantages.

[stacey-gammon]

Should we try somehow to make that the canonical way to do things? Seems like part of the problem with these tests is that there are so many ways to do the same thing, with some having hidden disadvantages.

Yep, I think so! Totally agree. A lot of the test code was written before testSubjects was around. A while back I added some stability functionality to testSubjects because of a bunch of stale element errors. So I def think find and testSubjects services are the way to go.

We might want to check in with @silne30 though because I synced with him a couple weeks ago and he had some alternative ideas to those services. He has a PR out with some upgrades that I haven't had a chance to look at yet. We should probably get his input to see if we can avoid creating a bunch of conflicts with his work while also tackling these flaky test issues.