test,win: disable test-tls-server-verify for CI #25284
Conversation
test-tls-server-verify takes a lont time to execute and times out on the Jenkins machines.
|
cc @joyent/node-coreteam : let's disable this test to unblock the CI. I am investigating the issue with P-1: #25279 |
|
+1
|
|
-1 from me. This test is passing like a charm on io.js. |
|
@indutny this PR doesn't actually disable the test in general, it just lets node-accept-pull-request ignore its failures. |
|
..that is, you need to pass --flaky-tests=dontcare to the test runner to see any difference. We only do that in node-accept-pull-request |
|
It is kind of the similar to disabling it, right? ;) what's the point if the test should be run by CI anyway? |
|
it's only disabling it in the context of the automated merge job. The alternative is to block accepting all other pull requests until this test is fixed. |
|
btw, the same problem seem to affect io.js: nodejs/node#1461. This test is slow only on Windows. |
test-tls-server-verify takes a lont time to execute and times out on the Jenkins machines. Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #25284
|
@orangemocha this is absolutely correct, but nevertheless it does not seem to be a reason to disable it, at least for me. |
|
@indutny does that mean you think we should stop accepting pull requests until this test is fixed? It has been slow on Windows forever, it just so happens that now it's crossing the 60 seconds mark. |
|
If the test is timing out because of a known system specific issue but we can verify that the test actually does pass independent of the automated run, then there should be no problem here. Omitting it from the automated pass should not be a problem. However, there should be a plan to address the issue and get the test re-enabled so we need to make sure that this is revisited. |
|
So just a note: io.js prefers to mentally "ignore" errors rather than have a "green" CI run with ignored tests. (I.e. having a not-CI-green build is OK in these instances and more transparent.) I have a feeling this error was fixed on our end by a recent jenkins upgrade, although I may be mistaken. |
|
It's entirely possible. I've seen these kinds of timeout errors magically disappear across jenkins versions before. In this case, the jenkins job is not able to simple ignore the error that occurs. |
|
@jasnell @Fishrock123 See my comment:
|
|
Right, I'm not saying this isn't slow, but I think the entire CI-must-be-100%-"green" / not-"blocked" mantra may be unsavoury / detrimental. (Admittedly, finding actual errors is easier when jenkins isn't trolling us, but still.) |
|
I just wanted to clarify that the issue is probably not due to Jenkins, since the test takes 80 seconds to complete when I log in with remote desktop on the machine and I run this single test manually. |
|
@Fishrick123 Yes, I agree that it's still not entirely clear what is more practical. However I think what @jasnell said previously would work well until this issue is fixed:
With an emphasis on: but we can verify that the test actually does pass independent of the automated run. |
@Fishrock123 , the point is not for the entire CI to be green. All other CI jobs except for node-accept-pull-request will show red when this test fails. Even node-accept-pull-request will not be green. It will be yellow, which gives us a clue that not everything is ok. The test is marked as "not ok", but there's a TODO next to it, which tells node-accept-pull-request to continue with the merge. |
|
Anyway, this PR already landed after James' +1 because I thought it was urgent to unblock the CI. Since it ended up being a bit controversial, we can discuss it at the next core team meeting, and decide then if it should stay or be reverted. |
|
Landed in 0dbaee4 |
|
Thank you @orangemocha 👍 I think that with some extra care until the test is fixed, this is the best solution for now. |
test-tls-server-verify takes a lont time to execute and times
out on the Jenkins machines.