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.

Sign up for GitHub

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

Jump to bottom

Fixes for projects/user pages in Openshift #9530

Merged
merged 1 commit into from
Oct 17, 2018
Merged

Fixes for projects/user pages in Openshift #9530

merged 1 commit into from
Oct 17, 2018

Conversation

stefwalter
Copy link
Contributor

@stefwalter stefwalter commented Jun 28, 2018 •
edited by martinpitt
Loading

These are fixes and cosmetic touch ups for the projects page in Openshift.

@stefwalter
Copy link
Contributor Author

Some examples of broken styles and layout, that this pull request fixes:

screenshot from 2018-06-28 08-44-17
screenshot from 2018-06-28 09-01-56
screenshot from 2018-06-28 09-04-05

@stefwalter stefwalter force-pushed the fix-openshift-tests branch from 34965ce to 4bbb2b5 Compare June 28, 2018 09:54
@stefwalter stefwalter changed the title Fixes for openshift project tests Fixes for openshift tests Jun 28, 2018
@stefwalter stefwalter changed the title Fixes for openshift tests WIP: Fixes for openshift tests Jun 28, 2018
@stefwalter stefwalter force-pushed the fix-openshift-tests branch from 4bbb2b5 to e71e754 Compare June 28, 2018 12:14
martinpitt
martinpitt reviewed Jun 28, 2018
View reviewed changes
test/verify/check-openshift Outdated
@@ -542,15 +542,15 @@ LABEL io.projectatomic.nulecule.atomicappversion="0.1.11" \
b.click("tr[data-row-id='vm-fedoravm']") # expand row
b.wait_present("tr[data-row-id='vm-fedoravm'] + tr.listing-ct-panel")
b.wait_present("#vm-fedoravm-node")
wait(lambda: b.text("#vm-fedoravm-node > a").strip() != "-") # `-` == unassigned ; so wait for a change
b.wait_js_cond('$("#vm-fedoravm-node > a").text().strip() !== "-"') # `-` == unassigned ; so wait for a change
Copy link
Member

Choose a reason for hiding this comment

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

This is indeed a much more efficient way to wait for a condition, as it doesn't need to keep circling between Python, CDP, and the browser; but in principle it should achieve the same thing. The browser does continue to run the page while talking (or not) via CDP to it. Not running the page was a PhantomJS behaviour. Does that correspond to any test flake?

@martinpitt martinpitt added the question Further information is requested label Jul 5, 2018
@martinpitt
Copy link
Member

@stefwalter , is this still WIP, or can/should we land some of this?

@stefwalter
Copy link
Contributor Author

These tests have regressed to the point of not even being able to run them once through on my own machine ...

@stefwalter stefwalter added the blocked Don't land until something else happens first (see task list) label Jul 6, 2018
martinpitt
martinpitt previously approved these changes Jul 20, 2018
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

These all look good to me. As I said I don't actually think that the last commit actually fixes a flake, it's just optimization. But it's a nice little cleanup anyway. But this should be rebased to current master to get some up to date test runs.

@martinpitt martinpitt force-pushed the fix-openshift-tests branch from 91f8b01 to 501edba Compare July 20, 2018 09:01
@martinpitt martinpitt removed needs-rebase question Further information is requested blocked Don't land until something else happens first (see task list) labels Jul 20, 2018
@martinpitt
Copy link
Member

I pushed a rebase, and took out the last commit for now. It's unrelated, and introducing jQuery there doesn't leave me with lots of confidence, so let's revisit that part in a separate PR. I'm running openshift test locally with that now, but let's see what the bots have to say.

@martinpitt
Copy link
Member

Running check-openshift on my laptop, with just -j2.

On master, everything succeeded except for TestRegistry.testImagestreamImport, which worked on a retry.

On this branch, there are three failures:

  • TestRegistry.testImagestreamImport like above, which worked on a retry.
  • TestRegistry.testProjectUsers failed several times in a row on b.click(".dropdown-menu a[value='testprojectuserproj']" not being present. I tried to add a wait_present() for it, but that doesn't help.
  • TestRegistry.testProjectGroups has literally tons of errors like below, and fails on them persistently, too:
> error: TypeError: Cannot read property '0' of null
    at $parseFunctionCall (http://127.0.0.2:9091/cockpit/$8aae564ef09cc16c6fdec180e0d3c986bea4bb5f7adaee4b9a448892c79ad82a/kubernetes/registry.js:12531:23)
    at Object.regularInterceptedExpression (http://127.0.0.2:9091/cockpit/$8aae564ef09cc16c6fdec180e0d3c986bea4bb5f7adaee4b9a448892c79ad82a/kubernetes/registry.js:13036:22)
    at Scope.$digest (http://127.0.0.2:9091/cockpit/$8aae564ef09cc16c6fdec180e0d3c986bea4bb5f7adaee4b9a448892c79ad82a/kubernetes/registry.js:14460:41)
    at http://127.0.0.2:9091/cockpit/$8aae564ef09cc16c6fdec180e0d3c986bea4bb5f7adaee4b9a448892c79ad82a/kubernetes/registry.js:14665:27
    at completeOutstandingRequest (http://127.0.0.2:9091/cockpit/$8aae564ef09cc16c6fdec180e0d3c986bea4bb5f7adaee4b9a448892c79ad82a/kubernetes/registry.js:4996:11)
    at http://127.0.0.2:9091/cockpit/$8aae564ef09cc16c6fdec180e0d3c986bea4bb5f7adaee4b9a448892c79ad82a/kubernetes/registry.js:5384:8

So this whole PR actually makes things worse. I'll split this up into several smaller PRs.

@martinpitt martinpitt mentioned this pull request Jul 20, 2018
Merged
@martinpitt martinpitt added the blocked Don't land until something else happens first (see task list) label Jul 20, 2018
@martinpitt martinpitt force-pushed the fix-openshift-tests branch from 501edba to d135623 Compare July 20, 2018 10:24
martinpitt pushed a commit to stefwalter/cockpit that referenced this pull request Jul 20, 2018
@stefwalter @martinpitt
kubernetes: Load appropriate kubernetes objects for add member dialogs
d135623 
These dialogs need to ask Kuberenetes for data, and not expect side
effects of other pages to load the data for them.

Closes cockpit-project#9530
@martinpitt
Copy link
Member

I split out the six "safe" ones to #9701. The two additional ones ("kubernetes: Load appropriate kubernetes objects for add member dialogs" and "kubernetes: Populate the user/group 'Add Member' drop down on the fly") cause the above crashes and testProject{Users,Groups}() regressions.

I rebased this PR on top of #9701 to expose the regressions on the bots, and keep the needswork label.

@martinpitt martinpitt dismissed their stale review July 20, 2018 10:25

causes regressions, see above

@martinpitt martinpitt removed the blocked Don't land until something else happens first (see task list) label Sep 4, 2018
@stefwalter
kubernetes: Populate the user/group 'Add Member' drop down on the fly
ba956c6 
This allows data arriving from kubernetes to populate this dropdown
on the projects page 'Add Member' dialog. This prevents races in the
tests.

We had to change a bit more code than expected due to the fact that
Angular wants to have stable objects across digests.

Load appropriate kubernetes objects for add member dialogs

These dialogs need to ask Kuberenetes for data, and not expect side
effects of other pages to load the data for them.
@stefwalter stefwalter changed the title WIP: Fixes for openshift tests Fixes for projects/user pages in Openshift Oct 16, 2018
martinpitt
martinpitt approved these changes Oct 17, 2018
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

I can't say I truly understand this, but it looks plausible and demonstrably works. Thanks!

@martinpitt martinpitt merged commit 4201b5c into cockpit-project:master Oct 17, 2018
@stefwalter stefwalter deleted the fix-openshift-tests branch December 5, 2018 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinpitt martinpitt martinpitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stefwalter @martinpitt