Add RobustHTTPClient tests #176
Conversation
Also revises the existing test to use example.com instead of x.com since there is now a site with the x.com domain name. Switched to hamcrest assertions because I'm more comfortable with their output.
MarkEWaite
changed the title
| @Test | ||
| @WithoutJenkins |
There was a problem hiding this comment.
I am not sure this does anything (useful) in conjunction with @ClassRule. Better to separate unit from functional tests in distinct source files.
There was a problem hiding this comment.
Will do that. Thanks
There was a problem hiding this comment.
Done in b2a588a. Thanks very much!
| final AssertionError e = assertThrows(AssertionError.class, () -> { | ||
| RobustHTTPClient.sanitize(new URL("https://example.com/ /has/space/in/url/")); |
There was a problem hiding this comment.
This is misleading. https://github.com/jenkinsci/apache-httpcomponents-client-4-api-plugin/blob/master/src/main/java/io/jenkins/plugins/httpclient/RobustHTTPClient.java#L261-L264 only throws an assertion error when assertions are enabled, which they would not be in production. Perhaps better to just switch this to a warning. (Unfortunately URL allows bogus strings which URI does not.)
There was a problem hiding this comment.
Thanks. I've added two tests in b2a588a, one with assertions enabled and one with assertions disabled. I've confirmed that the tests pass as expected whether assertions are enabled or disabled.
| @Test | ||
| public void testDownloadFile() throws Exception { | ||
| JenkinsRule.WebClient wc = j.createWebClient(); | ||
| RobustHTTPClient client = wc.executeOnServer(() -> { |
There was a problem hiding this comment.
RobustHTTPClient client = new RobustHTTPClient() without the executeOnServer throws an exception that the object is not being constructed in the Jenkins controller JVM. This is my "cheat" to cause the constructor to think that it is being called on the controller JVM.
There was a problem hiding this comment.
Not sure what you were doing, but try it again. Unless you are using RealJenkinsRule, or running tests in e.g. JenkinsSessionRule at a time when the server is not started, code run in the Surefire test will be in the same JVM and so
package io.jenkins.plugins.httpclient;
import org.junit.ClassRule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
public class XTest {
@ClassRule public static JenkinsRule r = new JenkinsRule();
@Test public void x() {
new RobustHTTPClient();
}
}passes as expected.
There was a problem hiding this comment.
Thanks very much! Fixed in b2a588a
| RobustHTTPClient client = wc.executeOnServer(() -> { | ||
| RobustHTTPClient internalClient = new RobustHTTPClient(); | ||
| internalClient.setStopAfterAttemptNumber(1); | ||
| internalClient.setTimeout(50, TimeUnit.MICROSECONDS); |
There was a problem hiding this comment.
This looks flaky. Set a real timeout, and instead of downloading the Jenkins root URL, use a @TestExtension on an UnprotectedRootAction whose doIndex sleeps.
There was a problem hiding this comment.
Thanks. I like that idea much better.
There was a problem hiding this comment.
I won't have time to implement that change immediately, since it will require that I learn more about @TestExtension and UnprotectedRootAction. I'll leave this pull request open until I have time to implement that.
There was a problem hiding this comment.
Roughly, and from memory:
@Test public void xxx() {
theClient.download(r.getRootURL() + "hangs/");
}
@TestExtension("xxx") public static final class Hangs extends InvisibleAction implements UnprotectedRootAction {
@Override public String getUrl() {return "hangs";}
HttpResponse public void doIndex() {Thread.sleep(Long.MAX_VALUE);}
}There was a problem hiding this comment.
Implemented in c21f9c1
Creates a new test that checks santiize() behavior if assertions are disabled. The test is executed if Maven is called with: mvn clean -DenableAssertions=false verify Simplify the JenkinsRule based integration test thanks to feedback from @jglick. No need to use a WebClient to wrap the execution of the constructor.
| assertThat( | ||
| RobustHTTPClient.sanitize(new URL("http://example.com/some/long/path")), | ||
| is("http://example.com/some/long/path")); |
There was a problem hiding this comment.
Also revises the existing test to use example.com instead of x.com since there is now a site with the x.com domain name.
this is at odds here, if the rationale here was that x.com now exists so use something that does not exist, then we can not use example.com as that exists (and is reserved). in other words you should use a different reserved domain (or TLD) that is both reserved and should not exist.
e.g. foo.invalid
There was a problem hiding this comment.
The switch from x.com to example.com had multiple motivations. I didn't mention all the motivations in the commit message. I should have done that.
It was intended to intentionally switch to a domain name that is reserved for documentation purposes. The test is documenting that the sanitization behaves as expected.
It was intended to make the URL string longer so that all three of the assertions the tests would format the same way.
It was intended to stop using a domain that now exists and is a redirect to twitter.com.
I'm fine with switching to another domain if that makes the test clearer.
There was a problem hiding this comment.
It was intended to intentionally switch to a domain name that is reserved for documentation purposes. The test is documenting that the sanitization behaves as expected.
I'm not sure I would call a test documentation according to the standard :)
if we do not care that a domain could or could not exist for this, then .test is available for this purpose (it may return NXDOMAIN like .invalid, but it may also be defined locally for testing purposes)
if we need a domain to exist (ie something will fail otherwise) then there is no difference here between x.com and example.com or even jenkins.io (with the exception that we control jenkins.io).
this is more an FYI.
There was a problem hiding this comment.
I think example.com is fine for this purpose.
|
|
||
| @Test | ||
| public void sanitizeReturnsInvalidURLs() throws Exception { | ||
| Assume.assumeFalse(RobustHTTPClient.class.desiredAssertionStatus()); |
There was a problem hiding this comment.
Fine, though again I would suggest just switching production code to not use assert here if it is tricky to test.
Add RobustHTTPClient tests
Also revises the existing test to use example.com instead of x.com since there is now a site with the x.com domain name.
Switched to hamcrest assertions because I'm more comfortable with their output.
I'd appreciate a review from either @dwnusbaum or @jglick in case there is some mistake that I've made. Not required, but would love to confirm that the technique I've used is a reasonable technique.
Testing done
Tests compile and run successfully with Java 11 on Linux. Rely on ci.jenkins.io to check Windows.
Submitter checklist