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

[Archive Migration] x-pack-banners/multispace #135783

Merged

Conversation

wayneseymour
Copy link
Member

@wayneseymour wayneseymour commented Jul 6, 2022

Summary

I've the before() fn loading the new kbn archive
containing one config, for the default space.
Then, the fn creates a new space and changes the ui settings of this new space,
w/o using an archive.

Helps with: #102552

Bugs Filed:
#140307
#140309

@wayneseymour wayneseymour added Team:QA Team label for QA Team test_xpack_functional release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.4.0 labels Jul 6, 2022
@wayneseymour wayneseymour self-assigned this Jul 6, 2022
@wayneseymour wayneseymour marked this pull request as ready for review July 6, 2022 14:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-qa (Team:QA)

@wayneseymour wayneseymour requested review from pgayvallet, LeeDr, marius-dr and cuff-links and removed request for cuff-links, marius-dr and LeeDr July 6, 2022 14:07
@LeeDr
Copy link

LeeDr commented Jul 7, 2022

@wayneseymour I don't think that kbn_archive really does anything for this test. It only contains a config saved object and Kibana would have already created a config object when it starts (and will ignore the one you load).

@LeeDr
Copy link

LeeDr commented Jul 7, 2022

I'm trying it locally without the kbn_archive

@LeeDr
Copy link

LeeDr commented Jul 7, 2022

Interesting that this test won't pass for me locally. It fails here because the save button doesn't appear on the page after changing the banner text in advanced settings;

await PageObjects.settings.setAdvancedSettingsTextArea(
        'banners:textContent',
        'default space banner text'
      );

@LeeDr
Copy link

LeeDr commented Jul 7, 2022

I'm also getting tons of this error in the server log while running the test. I don't see any reason for this test to change the default route for the space. It doesn't seem to have anything to do with the test.

 proc [kibana] [2022-07-07T14:09:55.579-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:09:55.579-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:09:55.847-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:10:00.326-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 info [o.e.x.s.a.r.TransportPutRoleAction] [ftr] updated role [system_indices_superuser]
 info [o.e.x.s.a.u.TransportPutUserAction] [ftr] updated user [system_indices_superuser]
 info [o.e.x.s.a.u.TransportPutUserAction] [ftr] added user [test_user]
 proc [kibana] [2022-07-07T14:11:38.164-05:00][INFO ][plugins.security.routes] Logging in with provider "basic" (basic)
 proc [kibana] [2022-07-07T14:11:38.837-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:11:38.889-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:11:40.261-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:11:40.262-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:11:40.262-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:11:40.263-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:11:50.436-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..
 proc [kibana] [2022-07-07T14:11:50.458-05:00][WARN ][ui-settings-service] Ignore invalid UiSettings value. Error: [validation [defaultRoute]]: Must be a relative URL..

@LeeDr
Copy link

LeeDr commented Jul 7, 2022

I'm not sure how the test passes in CI, but locally it doesn't seem to work as the tests describe. The difference is mainly on the last test 'displays the global banner on the login page' where instead of displaying the global banner, it actually displays the default space banner.

Steps to reproduce:

  1. Start Elasticsearch and Kibana with node scripts/functional_tests_server.js --config x-pack/test/banners_functional/config.ts
    (the config file sets
    '--xpack.banners.placement=top',
    '--xpack.banners.textContent="global banner text"
    )
  2. log in to Kibana as elastic user. I see the global banner text as expected. I'm not really why I see the space selector since there's only the default space at this time.
  3. click on the default space
  4. click on the D and Manage spaces and create a new space space1
  5. go to Advanced Settings (still in the default space)
  6. change banners:textContent to default space banner, save changes, reload page, see the new banner text
  7. switch to space1, see global banner text
  8. logout and log back in
  9. on the space selector page, the banner is default space banner not global banner text

Either this is a bug, or the test is incorrect?

@LeeDr
Copy link

LeeDr commented Jul 7, 2022

It appears that these tests are not running in buildkite. If you go to the passing buildkite job and look at

Pick Test Group Run Order. .buildkite/scripts/steps/test/pick_test_group_run_order.sh

and then at
ftr_run_order.json

and then look for the test pathx-path/test/banners_functional it's not there.

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.

buildkite doesn't seem to be running these tests. There's some ftr config somewhere that it's probably missing from.

It's in the section here labeled # Configs that exist but weren't running in CI when this file was introduced

https://github.com/elastic/kibana/blob/main/.buildkite/ftr_configs.yml#L52

@LeeDr LeeDr added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc and removed Team:QA Team label for QA Team labels Jul 7, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@wayneseymour
Copy link
Member Author

buildkite doesn't seem to be running these tests. There's some ftr config somewhere that it's probably missing from.

It's in the section here labeled # Configs that exist but weren't running in CI when this file was introduced

https://github.com/elastic/kibana/blob/main/.buildkite/ftr_configs.yml#L52

Yeah, now I see it is defined within the disabled stanza of ftr_configs.yml

@LeeDr
Copy link

LeeDr commented Jul 8, 2022

Looks like @pgayvallet added these tests in #94449 a little over a year ago. Things have change since then quite a bit from Jenkins to Buildkite and how we run tests. But I think there still would have been some config file change to add the banners_functional tests to CI that I don't see in that PR. So I think these never ran in CI.
The settings_page method setAdvancedSettingsTextArea was also added in that PR and doesn't appear to be used anywhere else. I had problems with it locally but I think I have a fix for it (sending the return key to it after entering the text).
And then there's the actual functionality in the product doesn't seem to match the test descriptions. It could be a bug.

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Just putting a safety block (given I previously approved) so we don't forget about that:

x-pack/test/banners_functional/tests/index.ts Outdated Show resolved Hide resolved
@wayneseymour wayneseymour requested a review from a team as a code owner September 14, 2022 08:48
@wayneseymour
Copy link
Member Author

This should be reverted once #140688 get merged

I could have sworn this test was a duplicate. Am I crazy? lol

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.

Canyou change back from global_banner_text to global banner text as was originally in the test?

@pgayvallet
Copy link
Contributor

Can you change back from global_banner_text to global banner text as was originally in the test?

Nope. See #140688 (comment)

@LeeDr
Copy link

LeeDr commented Sep 21, 2022

Can you change back from global_banner_text to global banner text as was originally in the test?

Nope. See #140688 (comment)

Couldn't this PR just add the double-quotes here and include in this PR? https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/bin/scripts/kibana#L29

Or we want to do that in a separate PR?

@pgayvallet
Copy link
Contributor

Couldn't this PR just add the double-quotes here and include in this PR?

We can, yes (even if I'm not sure to see the issue using a string without whitespaces in our FTR tests)

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

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM when the duplicate test suite has been removed from .buildkite/ftr_configs.yml

.buildkite/ftr_configs.yml Outdated Show resolved Hide resolved
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @wayneseymour

@wayneseymour wayneseymour merged commit 3edba25 into elastic:main Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc test_xpack_functional v8.4.0 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants