Temporarily disable test with failing http connection #9333
Conversation
KellenSunderland
force-pushed
the
disable_r_test
branch
from
1739b77
to
ca74db5
Compare
| @@ -172,6 +172,7 @@ test_that("Fine-tune", { | |||
| }) | |||
|
|
|||
| test_that("Matrix Factorization", { | |||
| skip("Disabled due to an unavailible http server. Tracked here: http://bit.ly/2qybsto") | |||
There was a problem hiding this comment.
Please link the issue directly
There was a problem hiding this comment.
I'm not opposed, but can you provide some motivation for the change? I shortened it to avoid line limits and maintain readability.
Disable test "Matrix Factorization" from the R test_model xpackage.
|
I personally don't like to click at randomly shortened links on the
internet as I don't know where they are going to lead. Also, if I'm trying
to grep for all issues in the code, this one is going to get missed. The
meaning of the link is only available in it's context opposed to showing it
just by looking at the link itself.
|
KellenSunderland
force-pushed
the
disable_r_test
branch
from
ca74db5
to
873ce60
Compare
|
Would you be ok with a GitHub only shortener? Updated the PR. I'll also link the issue from the PR and vice-versa to avoid the need to grep to find disabled tests. |
|
This fixes the issue about untrusted links but still hides the source of the issue. I'd like to see whether this issue is tracked in MXNet, mshadow or some third party library. In my opinion, the information loss is not justified just in order to save a few characters - you could compare it with using meaningful variable names instead of var 1 ,var2 etc. |
|
Good points Marco. I prefer explaining the issue in the comment and providing the link just as a reference. I don't feel strongly about it, so let's see what the other reviewers say. |
|
I'm merging this in for now as the test is blocking the CI for other PRs and it's urgently needed to unblock development. |
|
Thanks Sheng.
On Jan 8, 2018 11:37 PM, "Sheng Zha" <notifications@github.com> wrote:
Merged #9333 <#9333>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9333 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AHGTE1qloNINnfm1aqcdwstnugoOhLJ3ks5tIpjFgaJpZM4RVdNc>
.
|
|
No problem. I merged this out of necessity. Regarding the links, I'm with Marco on having self-explanatory comments and links when possible. In that sense, a comment like "#9332" may work better than a shortened link because everyone knows how to find that issue. Let's keep it readable next time. |
|
Roger, I'll update when I can.
…On Mon, Jan 8, 2018 at 11:45 PM, Sheng Zha ***@***.***> wrote:
No problem. I merged this out of necessity. Regarding the links, I'm with
Marco on having self-explanatory comments and links when possible. In that
sense, a comment like "apache/incubator-mxnet#9332" may work better than a
shortened link because everyone knows how to find that issue. Let's keep it
readable next time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9333 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHGTE0n7kIr7LMdrMwHYVrmiMOcq6ushks5tIpqEgaJpZM4RVdNc>
.
|
|
Thanks @szha. Thanks @KellenSunderland for retriggering the PRs! |
|
Grouplens.org just got back online - that's timing! |
…#9333)" (apache#9379) This reverts commit 8ad77f3.
Disable test "Matrix Factorization" from the R test_model xpackage.
…#9333)" (apache#9379) This reverts commit 8ad77f3.
Disable test "Matrix Factorization" from the R test_model xpackage.
…#9333)" (apache#9379) This reverts commit 8ad77f3.
Disable test "Matrix Factorization" from the R test_model xpackage.
…#9333)" (apache#9379) This reverts commit 8ad77f3.
Disable test Matrix Factorization from the R package.
Description
This PR disables another test which is failing due to a remote http call. The site it's attempting to reach seems to be down globally.
Applicable Issue: #9332