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

test: Add tests for CPU and memory graph pages #13439

Merged
merged 1 commit into from
Jan 25, 2020

Conversation

martinpitt
Copy link
Member

@martinpitt martinpitt commented Jan 23, 2020

This makes sure that the page configures as expected and doesn't crash.

Don't test the innards of the graph again, as they are exactly the same
widgets as the ones already tested further above.

Reproduces https://bugzilla.redhat.com/show_bug.cgi?id=1792623

@martinpitt
Copy link
Member Author

This will fail until landing PR #13438

@martinpitt martinpitt added the blocked Don't land until something else happens first (see task list) label Jan 23, 2020
@martinpitt
Copy link
Member Author

The Oops on the graphs page is picked up by this new test as expected.

@martinpitt martinpitt added needswork and removed blocked Don't land until something else happens first (see task list) labels Jan 23, 2020
@martinpitt
Copy link
Member Author

Needs adjustment at least on fedora-coreos, as that doesn't have swap and thus uses a different memory graph.

@martinpitt
Copy link
Member Author

Broken on rhel-8-2-distropkg, so I pushed a fix to skip the test there until the fix lands in our image.

b.click("#link-cpu")
b.wait_not_visible(".ct-graph-memory")
b.wait_visible("#cpu_status_title")
b.wait_visible("#cpu_status_graph")
Copy link
Member

Choose a reason for hiding this comment

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

I thought that we are also going to check that the height (and width) is some reasonable number. Something like:

width = b.call_js_func('(function (sel) { return ph_find(sel).offsetWidth; })', "#cpu_status_graph")

Copy link
Member Author

Choose a reason for hiding this comment

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

Cheers! I pushed the size check.

marusak
marusak previously approved these changes Jan 24, 2020
Copy link
Member

@marusak marusak left a comment

Choose a reason for hiding this comment

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

Tests have the final word, but it looks good, thanks!
Just one comment, but it still does not check the graphs, so good as is.

This makes sure that the page configures as expected and doesn't crash.

Don't test the innards of the graph again, as they are exactly the same
widgets as the ones already tested further above. But do ensure that
they have a reasonable size.

Reproduces https://bugzilla.redhat.com/show_bug.cgi?id=1792623

Closes cockpit-project#13439
@martinpitt
Copy link
Member Author

@marusak: Can you please re-review? Thanks!

Copy link
Member

@marusak marusak left a comment

Choose a reason for hiding this comment

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

Thanks :)

@martinpitt martinpitt merged commit 33f7dcf into cockpit-project:master Jan 25, 2020
@martinpitt martinpitt deleted the graphs-test branch January 25, 2020 09:34
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.

2 participants