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

Explicit move to Applications Enabled page #1432

Merged
merged 1 commit into from
May 9, 2024

Conversation

diegolovison
Copy link
Contributor

@diegolovison diegolovison commented May 9, 2024

The test suite expect the product be on Applications Enabled page. Starting with RHOAI 2.10, we can have a new home page.

Jenkins: rhods-smoke/5371/

The test suite expect the product be on Applications Enabled page. Starting with RHOAI 2.10, we can have a new home page.
Copy link

sonarqubecloud bot commented May 9, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

github-actions bot commented May 9, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
454 0 0 454 100

@@ -80,6 +80,7 @@ Login To RHODS Dashboard
IF ${login-required} Login To Openshift ${ocp_user_name} ${ocp_user_pw} ${ocp_user_auth_type}
${authorize_service_account}= Is rhods-dashboard Service Account Authorization Required
IF ${authorize_service_account} Authorize rhods-dashboard service account
Navigate To Page Applications Enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

this is temporary right? we should check later that user ends up on the new home page

Copy link
Contributor Author

@diegolovison diegolovison May 9, 2024

Choose a reason for hiding this comment

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

The current test suite is expecting that. We just explicitly/write the code.
When we have a new home page we have two options:

  • rewrite the test suite in order to not expect the Enabled page by default
  • have a different test only for home page

Copy link
Contributor

@bdattoma bdattoma May 9, 2024

Choose a reason for hiding this comment

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

hm as far as I remember, tests use Wait For RHODS Dashboard To Load which allows you to set the expected page. So the tests don't wait explicitly for "Enabled" page, unless there are tests doing it explicitly, but don't know for sure

I found a dedicated discussion in Slack, let me go through it

Copy link
Contributor Author

@diegolovison diegolovison May 9, 2024

Choose a reason for hiding this comment

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

set the expected page

yes. but by default it is the Enabled

So the tests don't wait explicitly for "Enabled" page

they explicitly are expecting the Enabled page because of the login method

Copy link
Contributor

Choose a reason for hiding this comment

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

default can be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Later, yes. And I prefer not to have a default value.

Copy link
Member

Choose a reason for hiding this comment

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

So... is the time to revert this finally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suppose you are going to change something related to this code. Please make sure you do it explicitly.
In this case, if we read the code we know exactly what is happening.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I bumped into this when reviewing failures of some of our tests. I have two options basically:

  1. provide a bit ugly and easy solution (at least from my point of view) to change a couple of our tests only
  2. revert this workaround and check that no tests are broken which is much more work - but seem like a proper solution for me

And I would prefer to avoid both of the above of course... 🙃

Copy link
Member

Choose a reason for hiding this comment

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

FTR #1725

Copy link
Contributor

@FedeAlonso FedeAlonso left a comment

Choose a reason for hiding this comment

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

LGTM

@diegolovison diegolovison merged commit f9dfc88 into red-hat-data-services:master May 9, 2024
8 checks passed
jstourac added a commit to jstourac/ods-ci that referenced this pull request May 15, 2024
This started failing after this workaround red-hat-data-services#1432 ([1]). Originally the
test expected that after logout and another login, the user is
redirected to the original page where logout started. This doesn't
happen anymore because of this new redirect to `enabled` paged in the
mentioned workaround.

Once the workaround is removed, we can delete the line added in this PR
also. Though, the main point of the test isn't to check where the page
is open after login, but the actual login names work as expected only.

* [1] f9dfc88
jstourac added a commit that referenced this pull request May 15, 2024
This started failing after this workaround #1432 ([1]). Originally the
test expected that after logout and another login, the user is
redirected to the original page where logout started. This doesn't
happen anymore because of this new redirect to `enabled` paged in the
mentioned workaround.

Once the workaround is removed, we can delete the line added in this PR
also. Though, the main point of the test isn't to check where the page
is open after login, but the actual login names work as expected only.

* [1] f9dfc88
adolfo-ab pushed a commit to adolfo-ab/ods-ci that referenced this pull request May 21, 2024
This started failing after this workaround red-hat-data-services#1432 ([1]). Originally the
test expected that after logout and another login, the user is
redirected to the original page where logout started. This doesn't
happen anymore because of this new redirect to `enabled` paged in the
mentioned workaround.

Once the workaround is removed, we can delete the line added in this PR
also. Though, the main point of the test isn't to check where the page
is open after login, but the actual login names work as expected only.

* [1] f9dfc88
jiridanek pushed a commit to jstourac/ods-ci that referenced this pull request Aug 16, 2024
This removes the workaround added in [1,2] and expects the default
dashboard page to be the landing/home page now.

[1] red-hat-data-services#1432
[2] f9dfc88
jstourac added a commit that referenced this pull request Aug 16, 2024
This removes the workaround added in [1,2] and expects the default
dashboard page to be the landing/home page now.

[1] #1432
[2] f9dfc88
jgarciao pushed a commit to jgarciao/ods-ci that referenced this pull request Sep 1, 2024
This removes the workaround added in [1,2] and expects the default
dashboard page to be the landing/home page now.

[1] red-hat-data-services#1432
[2] f9dfc88
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.

4 participants