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

mgr/dashboard: Hosts Page Service Links Test #29516

Merged
merged 1 commit into from Aug 20, 2019
Merged

Conversation

adk3798
Copy link
Contributor

@adk3798 adk3798 commented Aug 6, 2019

Test links in "Services" column on hosts page properly link to Performance Counters pages

Fixes: https://tracker.ceph.com/issues/41142

Signed-off-by: Adam King adking@redhat.com
Signed-off-by: Rafael Quintero rquinter@redhat.com

@adk3798
Copy link
Contributor Author

adk3798 commented Aug 6, 2019

jenkins test dashboard

@adk3798 adk3798 changed the title mgr/dashboard: Hosts Page Service Links Tests mgr/dashboard: Hosts Page Service Links Test Aug 6, 2019
@adk3798
Copy link
Contributor Author

adk3798 commented Aug 6, 2019

jenkins test dashboard

@adk3798 adk3798 requested a review from a team August 9, 2019 13:07
Copy link

@Devp00l Devp00l left a comment

Choose a reason for hiding this comment

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

Tested it locally and it works 👍

src/pybind/mgr/dashboard/frontend/e2e/cluster/hosts.po.ts Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/frontend/e2e/cluster/hosts.po.ts Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/frontend/e2e/cluster/hosts.po.ts Outdated Show resolved Hide resolved
@adk3798
Copy link
Contributor Author

adk3798 commented Aug 9, 2019

Update:

  • Changed use of indexOf to includes when checking for a host to be present in table
  • Checks for a host being present and services being present in host were flipped and now stop running running the services link test (and fail) if either of those conditions don't hold. Also increases readability by having less indents.
  • Check for at least one link having been tested was moved from an if to an expect. Instead of printing out a message if no links were tested, the test now fails

src/pybind/mgr/dashboard/frontend/e2e/cluster/hosts.po.ts Outdated Show resolved Hide resolved
// check is any services links are present
services.getText().then((txt) => {
// check that text (links) is present in services box
if (txt.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use expect to validate this.

Suggested change
if (txt.length === 0) {
expect(txt.length).toBeGreaterThan(0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of adding in an expect expect to explicitly test this. However, I'd still like to leave the if block in. The reason being that the next line after the if block "const links = services.all(by.css('a.ng-star-inserted'));" will cause an Async Timeout error if no links are present (it seems to happen when you do a by css search on something that isn't there). Async Timeout errors will not only cause this test to fail but tend to also cause the next few tests to fail even if they should pass. To avoid that in the case of failure I think it would be fine to both have the explicit expect that tests that links are present and then also have the if to avoid the timeout that would occur afterwards if there are no links.

Copy link

Choose a reason for hiding this comment

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

It's also fine to just let this function end at this point as it will return a promise which is resolved in a function that expects if links_tested is greater than 0, which in this case will still be 0.

Test at least one host is present
Test links in "Services" column on hosts page properly link to Performance Counters pages

Fixes: https://tracker.ceph.com/issues/41142

Signed-off-by: Adam King <adking@redhat.com>
Signed-off-by: Rafael Quintero <rquinter@redhat.com>
@adk3798
Copy link
Contributor Author

adk3798 commented Aug 12, 2019

Update:

  • Removed redundant navigateTo from e2e
  • Added explicit test for presence of a host
  • Added expect that explicitly checks that services are present in first host

Copy link

@Devp00l Devp00l left a comment

Choose a reason for hiding this comment

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

Thanks for the update :) LGTM 👍

@LenzGr LenzGr merged commit aca9cf2 into ceph:master Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants