-
Notifications
You must be signed in to change notification settings - Fork 357
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
Broken travis from other repos #4921
Comments
If, possible, I would love to see the following timings:
Obviously it may not be possible to get all of that, but the more information we have the better we can solve this together. |
Red again...
PR merged: 2018-11-15 16:53 UTC fix created: 2018-11-16 14:10 UTC (assuming #4933) |
It took me about 15-20 minutes. And about 30 minutes until i realized that the tests are busted (don't know if that counts). Thanks for the fix @himdel ! |
This could be nicely automated by just storing some information about PR merges and travis build results from all the related repos. If the PR's build was green and the master branch afterwards went red, there's probably an issue in the other repositories. This is way too interesting to solve during working hours, if no one wants it, I might play with this during the holidays in December. |
@skateman That does sound like a fun project 😄 |
Another red... https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/474623058
(hidden in the diff, but expected has Comes from ManageIQ/manageiq#18266. Merged: 2019-01-02 15:17Z |
Thanks, now I know what broke providers-kubernetes tests, being subscribed to this issue saves a lot of time debugging 😉 |
I made a spreadsheet... link in the OP. |
Another.. https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/485263617
Caused by ManageIQ/manageiq#18378 Fixed... ? (added dates to spreadsheet) |
We just reverted ManageIQ/manageiq#18378 with ManageIQ/manageiq#18408, so I'm considering that "fixed" |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/498918915
Thanks @lpichler :) |
https://travis-ci.org/ManageIQ/manageiq/jobs/500425821#L2517-L2524
This issue tracks failures in the other direction too right? 😄 |
You mean because a UI PR was merged which broke core? |
Yes |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/519439431
cause ManageIQ/manageiq#18633 |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/builds/526150604
|
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/533531882#L2666
cause ManageIQ/manageiq#18778 |
Cause: ManageIQ/manageiq#18827 |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/544108049
Cause: ManageIQ/manageiq#18784 |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/544619532
( Cause: ManageIQ/manageiq#18687 |
@Fryguy I think the approach you outlined is a nice first step, but I don't expect it to have much effect by itself. It makes it easy to run the tests when you suspect there may be a problem, but... well, if a merger already suspects there may be a problem with a particular PR and doesn't try just running the tests locally, that's just negligence :). If the goal is to catch unexpected breakages, this does nothing, except perhaps serve as a basis for a policy of "do this for every PR where X,Y,Z". So.. what are the plans after that? |
It feels like the test mentioned in #4921 (comment) is in the wrong repo and should live with the YAML files that it's testing. |
Aah, yes.. it was moved together with So.. maybe we need to fix the UI spec to look at |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/562535976
ManageIQ/manageiq#18977 (comment) Cause: ManageIQ/manageiq#18977 |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/563014986
Cause: ManageIQ/manageiq#19053 |
Cause: ManageIQ/manageiq#19473 (comment) |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/611907845#L2080
(the expected Cause: ManageIQ/manageiq#19491 |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/614038504#L2079
Cause: ManageIQ/manageiq-providers-ovirt#421, ManageIQ/manageiq-providers-vmware#467 Fix: #6428 |
In this last case it was a provider change that caused the failure. I don't think that running the UI specs on every core PR and every provider PR is going to be very reasonable. |
Well, @agrare how this should work is that we have a set of provider methods that are guaranteed to have a stable interface, and test that UI and everything else just use those methods. Then there would be no need for this, you could just test those methods in provider repos. (IIRC that was even an explicit target at some point, we can't really have pluggable providers without it.) But, until that happens, the UI (and API) is the main place where provider code and core code come together. We are the integration. So, I think it's absolutely reasonable to run the UI tests everywhere. Of course, if we do that, we may want to go through those tests and comb them so that tests that won't catch any of these problems only run ui side. But, that's probably just JS tests now anyway. Really. I mean, we don't have to run tests that test our toolbar implementation in providers. That, or write actual shared provider tests that test that every method the UI needs keeps the same interface :). |
cc: @chessbyte |
We completely agree on this point, lets add moving provider specifics in UI specs to the pluggability checklist ManageIQ/manageiq#19440 so we can start moving the provider logic out of the UI. |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/615790473#L2060
Cause: ManageIQ/manageiq#19544 |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/620502649
Cause: ManageIQ/manageiq#19570 |
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/621722549
Cause: ManageIQ/manageiq#19602, ManageIQ/manageiq#19603 |
Created #6504 to track decoupling of UI tests from core operations methods |
|
Upgrading kubeclient broke stuff in the UI:
|
Hey @skateman I suspect this wasn't upgrading kubeclient or #7043 (since cross-repo tests were green on that one). I'm guessing it was probably ManageIQ/manageiq-providers-openshift#174 which changed how the method was internally constructed even though the API stayed the same. The test should probably be rewritten to not expect anything from the underlying kubeclient connection. |
@mzazrivec can you create a new spec according to this? I guess we need |
Cause: ManageIQ/manageiq#20361 |
Closing since we now have cross repo tests |
With some frequency, ui-classic travis is red because of changes made in other repos.
This issue exists to track those - please mention this issue in any PR which breaks ui-classic travis (or fixes such a break).
(Please do not use for travis failures not caused by other ManageIQ repos.)
(related to discussion in ManageIQ/manageiq#18173)
Spreadsheet: https://docs.google.com/spreadsheets/d/1V_o2vxaBsHlX70E0SOwoeArmXAZIfQQI8V_JBC8dGvE
The text was updated successfully, but these errors were encountered: