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.

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

Add RobustHTTPClient tests #176

Merged
merged 8 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
/*
* The MIT License
*
* Copyright 2018 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package io.jenkins.plugins.httpclient;

import static org.hamcrest.MatcherAssert.*;
import static org.hamcrest.Matchers.*;
import static org.junit.Assert.assertThrows;

import hudson.AbortException;
import hudson.model.TaskListener;
import hudson.model.UnprotectedRootAction;
import java.io.File;
import java.io.IOException;
import java.net.URL;
import java.nio.file.Files;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.TestExtension;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.HttpResponses;

public class RobustHTTPClientIntegrationTest {

@ClassRule
public static JenkinsRule j = new JenkinsRule();

@Rule
public TemporaryFolder tempFolder = new TemporaryFolder();

private RobustHTTPClient client;
private File f;

@Before
public void createRobustHTTPClient() throws Exception {
client = new RobustHTTPClient();
f = new File(tempFolder.newFolder(), "jenkins-index.html");
}

@Test
public void testDownloadFile() throws Exception {
client.downloadFile(f, j.getURL(), TaskListener.NULL);
assertThat(Files.readString(f.toPath()), containsString("<title>Dashboard [Jenkins]</title>"));
}

/**
* Wait no more than TIMEOUT seconds for response
*/
private static final int TIMEOUT = 2;

@Test
public void testDownloadFileWithTimeoutException() throws Exception {
client.setStopAfterAttemptNumber(1);
client.setTimeout(TIMEOUT, TimeUnit.SECONDS);
URL hangingURL = new URL(j.getURL().toString() + "intentionally-hangs-always");
final IOException e = assertThrows(IOException.class, () -> {
client.downloadFile(f, hangingURL, TaskListener.NULL);
});
assertThat(e.getCause(), isA(TimeoutException.class));
}

@Test
public void testDownloadFileWithRetry() throws Exception {
// Try up to three times, should succeed on second try
client.setStopAfterAttemptNumber(3);
// Wait 213 milliseconds before retry (no need for a long wait)
client.setWaitMultiplier(213, TimeUnit.MILLISECONDS);
// Wait no more than 3x the TIMEOUT between retries (upper bound)
client.setWaitMaximum(TIMEOUT * 3, TimeUnit.SECONDS);
// Retry if no response in TIMEOUT seconds
client.setTimeout(TIMEOUT, TimeUnit.SECONDS);
URL hangsOnceURL = new URL(j.getURL().toString() + "intentionally-hangs-once");
client.downloadFile(f, hangsOnceURL, TaskListener.NULL);
assertThat(Files.readString(f.toPath()), containsString("a-dubious-response-from-hangs-once"));
}

@Test
public void testDownloadFileFromNonExistentLocation() throws Exception {
URL badURL = new URL(j.getURL().toString() + "/page/does/not/exist");
final AbortException e = assertThrows(AbortException.class, () -> {
client.downloadFile(f, badURL, TaskListener.NULL);
});
assertThat(e.getMessage(), containsString("Failed to download "));
}

@Test
public void testDownloadFileStopAfterOneAttempt() throws Exception {
client.setStopAfterAttemptNumber(1);
URL badURL = new URL(j.getURL().toString() + "/page/does/not/exist");
final AbortException e = assertThrows(AbortException.class, () -> {
client.downloadFile(f, badURL, TaskListener.NULL);
});
assertThat(e.getMessage(), containsString("Failed to download "));
}

@TestExtension
public static class IntentionallyHangsAlwaysAction implements UnprotectedRootAction {

@Override
public String getIconFileName() {
return null;
}

@Override
public String getDisplayName() {
return null;
}

@Override
public String getUrlName() {
return "intentionally-hangs-always";
}

public HttpResponse doIndex() {
try {
// Sleep 3x longer than the timeout
Thread.sleep(TIMEOUT * 3L * 1000L);
} catch (InterruptedException ie) {
// Ignore the exception
}
return HttpResponses.text("a-dubious-response-from-hangs-always");
}
}

@TestExtension
public static class IntentionallyHangsOnceAction implements UnprotectedRootAction {

private int requestCount = 0;

@Override
public String getIconFileName() {
return null;
}

@Override
public String getDisplayName() {
return null;
}

@Override
public String getUrlName() {
return "intentionally-hangs-once";
}

public HttpResponse doIndex() {
requestCount++;
if (requestCount <= 1) {
/* Wait 3x longer than timeout on first request */
try {
// Sleep 3x longer than the timeout
Thread.sleep(TIMEOUT * 3L * 1000L);
} catch (InterruptedException ie) {
// Ignore the exception
}
}
return HttpResponses.text("a-dubious-response-from-hangs-once");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,48 @@

package io.jenkins.plugins.httpclient;

import static org.junit.Assert.*;
import static org.hamcrest.MatcherAssert.*;
import static org.hamcrest.Matchers.*;
import static org.junit.Assert.assertThrows;

import java.net.URL;
import org.junit.Assume;
import org.junit.Test;

public class RobustHTTPClientTest {

@Test
public void sanitize() throws Exception {
assertEquals("http://x.com/some/path", RobustHTTPClient.sanitize(new URL("http://x.com/some/path")));
assertEquals(
"https://x.com/some/path?…", RobustHTTPClient.sanitize(new URL("https://x.com/some/path?auth=s3cr3t")));
assertEquals(
"https://…@x.com/otherpath", RobustHTTPClient.sanitize(new URL("https://user:s3cr3t@x.com/otherpath")));
assertThat(
RobustHTTPClient.sanitize(new URL("http://example.com/some/long/path")),
is("http://example.com/some/long/path"));
Comment on lines +38 to +40
Copy link
Member

@jtnord jtnord Sep 11, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jtnord jtnord Sep 11, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think example.com is fine for this purpose.

assertThat(
RobustHTTPClient.sanitize(new URL("https://example.com/some/path?auth=s3cr3t")),
is("https://example.com/some/path?…"));
assertThat(
RobustHTTPClient.sanitize(new URL("https://user:s3cr3t@example.com/otherpath")),
is("https://…@example.com/otherpath"));
}

@Test
public void sanitizeReturnsInvalidURLs() throws Exception {
Assume.assumeFalse(RobustHTTPClient.class.desiredAssertionStatus());
Copy link
Member

Choose a reason for hiding this comment

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

Fine, though again I would suggest just switching production code to not use assert here if it is tricky to test.

/* URLs that cannot be converted to URIs are not sanitized.
* They are returned "as-is" when assertions are disabled.
*/
String invalidURI = "https://user:s3cr3t@example.com/ /has/space/in/url/";
assertThat(RobustHTTPClient.sanitize(new URL(invalidURI)), is(invalidURI));
}

@Test
public void sanitizeExceptionOnInvalidURLs() throws Exception {
Assume.assumeTrue(RobustHTTPClient.class.desiredAssertionStatus());
/* URLs that cannot be converted to URIs are not sanitized.
* An assertion exception is thrown when assertions are enabled.
*/
String invalidURI = "https://user:s3cr3t@example.com/ /has/space/in/url/";
final AssertionError e = assertThrows(AssertionError.class, () -> {
RobustHTTPClient.sanitize(new URL(invalidURI));
});
assertThat(e.getCause().getMessage(), containsString("Illegal character in path at index 32: " + invalidURI));
}
}