Skip to content

Commit

Permalink
Correctly refactor unit tests
Browse files Browse the repository at this point in the history 
The unit test refactoring left copy/pasted code.  This commit splits
the common piece into a single header, allowign `s3_tests.cc` with
the unit tests that utilize AWS S3 while `s3_unit_tests.cc` use the
minio instance started up by ctest.
  • Loading branch information
@bbockelm
bbockelm committed Dec 2, 2024
1 parent 87f5a7c commit 7e160ee
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 221 deletions.
11 changes: 9 additions & 2 deletions src/CurlUtil.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,13 @@ void CurlWorker::Run() {

std::vector<struct curl_waitfd> waitfds;
waitfds.resize(2);
// The `curl_multi_wait` call in the event loop needs to be interrupted when
// additional work comes into one of the two queues (either the global queue
// or the per-worker unpause queue). To do this, the queue objects will write
// to a file descriptor when a new HTTP request is ready; we add these FDs to
// the list of FDs for libcurl to poll in order to trigger a wakeup. The
// `Consume`/`TryConsume` methods will have a side-effect of reading from the
// pipe if a request is available.
waitfds[0].fd = queue.PollFD();
waitfds[0].events = CURL_WAIT_POLLIN;
waitfds[0].revents = 0;
Expand Down Expand Up @@ -237,9 +244,9 @@ void CurlWorker::Run() {
}
} catch (...) {
m_logger.Log(LogMask::Debug, "Run",
"Unable to setup the curl handle");
"Unable to set up the curl handle");
op->Fail("E_NOMEM",
"Failed to setup the curl handle for the operation");
"Failed to set up the curl handle for the operation");
continue;
}
m_op_map[curl] = op;
Expand Down
38 changes: 1 addition & 37 deletions test/s3_tests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,12 @@
*
***************************************************************/

#include "s3_tests_common.hh"
#include "../src/S3Commands.hh"
#include "../src/S3FileSystem.hh"
#include "../src/shortfile.hh"

#include <XrdOuc/XrdOucEnv.hh>
#include <XrdSys/XrdSysError.hh>
#include <XrdSys/XrdSysLogger.hh>
#include <gtest/gtest.h>

class TestAmazonRequest : public AmazonRequest {
Expand Down Expand Up @@ -65,41 +64,6 @@ TEST(TestS3URLGeneration, Test1) {
ASSERT_EQ(generatedHostUrl, "https://s3-service.com:443/test-object");
}

class FileSystemFixtureBase : public testing::Test {
protected:
FileSystemFixtureBase()
: m_log(new XrdSysLogger(2, 0)) // Log to stderr, no log rotation
{}

void SetUp() override {
setenv("XRDINSTANCE", "xrootd", 1);
char tmp_configfn[] = "/tmp/xrootd-s3-gtest.cfg.XXXXXX";
auto result = mkstemp(tmp_configfn);
ASSERT_NE(result, -1) << "Failed to create temp file ("
<< strerror(errno) << ", errno=" << errno << ")";
m_configfn = std::string(tmp_configfn);

auto contents = GetConfig();
ASSERT_FALSE(contents.empty());
ASSERT_TRUE(writeShortFile(m_configfn, contents, 0))
<< "Failed to write to temp file (" << strerror(errno)
<< ", errno=" << errno << ")";
}

void TearDown() override {
if (!m_configfn.empty()) {
auto rv = unlink(m_configfn.c_str());
ASSERT_EQ(rv, 0) << "Failed to delete temp file ("
<< strerror(errno) << ", errno=" << errno << ")";
}
}

virtual std::string GetConfig() = 0;

std::string m_configfn;
std::unique_ptr<XrdSysLogger> m_log;
};

class FileSystemS3VirtualBucket : public FileSystemFixtureBase {
protected:
virtual std::string GetConfig() override {
Expand Down
64 changes: 64 additions & 0 deletions test/s3_tests_common.hh
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/***************************************************************
*
* Copyright (C) 2024, Pelican Project, Morgridge Institute for Research
*
* Licensed under the Apache License, Version 2.0 (the "License"); you
* may not use this file except in compliance with the License. You may
* obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
***************************************************************/

#include "../src/shortfile.hh"

#include <XrdSys/XrdSysLogger.hh>
#include <gtest/gtest.h>

#include <memory>
#include <string>

#include <stdlib.h>
#include <string.h>
#include <unistd.h>

class FileSystemFixtureBase : public testing::Test {
protected:
FileSystemFixtureBase()
: m_log(new XrdSysLogger(2, 0)) // Log to stderr, no log rotation
{}

void SetUp() override {
setenv("XRDINSTANCE", "xrootd", 1);
char tmp_configfn[] = "/tmp/xrootd-s3-gtest.cfg.XXXXXX";
auto result = mkstemp(tmp_configfn);
ASSERT_NE(result, -1) << "Failed to create temp file ("
<< strerror(errno) << ", errno=" << errno << ")";
m_configfn = std::string(tmp_configfn);

auto contents = GetConfig();
ASSERT_FALSE(contents.empty());
ASSERT_TRUE(writeShortFile(m_configfn, contents, 0))
<< "Failed to write to temp file (" << strerror(errno)
<< ", errno=" << errno << ")";
}

void TearDown() override {
if (!m_configfn.empty()) {
auto rv = unlink(m_configfn.c_str());
ASSERT_EQ(rv, 0) << "Failed to delete temp file ("
<< strerror(errno) << ", errno=" << errno << ")";
}
}

virtual std::string GetConfig() = 0;

std::string m_configfn;
std::unique_ptr<XrdSysLogger> m_log;
};
183 changes: 1 addition & 182 deletions test/s3_unit_tests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,13 @@
// meaning no internet connectivity is needed to run them.
//

#include "s3_tests_common.hh"
#include "../src/S3Commands.hh"
#include "../src/S3File.hh"
#include "../src/S3FileSystem.hh"
#include "../src/shortfile.hh"

#include <XrdOuc/XrdOucEnv.hh>
#include <XrdSys/XrdSysError.hh>
#include <XrdSys/XrdSysLogger.hh>
#include <gtest/gtest.h>

#include <fcntl.h>
Expand Down Expand Up @@ -71,186 +70,6 @@ void parseEnvFile(const std::string &fname) {
}
}

class TestAmazonRequest : public AmazonRequest {
public:
XrdSysLogger log{};
XrdSysError err{&log, "TestS3CommandsLog"};

TestAmazonRequest(const std::string &url, const std::string &akf,
const std::string &skf, const std::string &bucket,
const std::string &object, const std::string &path,
int sigVersion)
: AmazonRequest(url, akf, skf, bucket, object, path, sigVersion, err) {}

// For getting access to otherwise-protected members
std::string getHostUrl() const { return hostUrl; }
};

TEST(TestS3URLGeneration, Test1) {
const std::string serviceUrl = "https://s3-service.com:443";
const std::string b = "test-bucket";
const std::string o = "test-object";

// Test path-style URL generation
TestAmazonRequest pathReq{serviceUrl, "akf", "skf", b, o, "path", 4};
std::string generatedHostUrl = pathReq.getHostUrl();
ASSERT_EQ(generatedHostUrl,
"https://s3-service.com:443/test-bucket/test-object");

// Test virtual-style URL generation
TestAmazonRequest virtReq{serviceUrl, "akf", "skf", b, o, "virtual", 4};
generatedHostUrl = virtReq.getHostUrl();
ASSERT_EQ(generatedHostUrl,
"https://test-bucket.s3-service.com:443/test-object");

// Test path-style with empty bucket (which we use for exporting an entire
// endpoint)
TestAmazonRequest pathReqNoBucket{serviceUrl, "akf", "skf", "",
o, "path", 4};
generatedHostUrl = pathReqNoBucket.getHostUrl();
ASSERT_EQ(generatedHostUrl, "https://s3-service.com:443/test-object");
}

class FileSystemFixtureBase : public testing::Test {
protected:
FileSystemFixtureBase()
: m_log(new XrdSysLogger(2, 0)) // Log to stderr, no log rotation
{}

void SetUp() override {

setenv("XRDINSTANCE", "xrootd", 1);
char tmp_configfn[] = "/tmp/xrootd-s3-gtest.cfg.XXXXXX";
auto result = mkstemp(tmp_configfn);
ASSERT_NE(result, -1) << "Failed to create temp file ("
<< strerror(errno) << ", errno=" << errno << ")";
m_configfn = std::string(tmp_configfn);

auto contents = GetConfig();
ASSERT_FALSE(contents.empty());
ASSERT_TRUE(writeShortFile(m_configfn, contents, 0))
<< "Failed to write to temp file (" << strerror(errno)
<< ", errno=" << errno << ")";
}

void TearDown() override {
if (!m_configfn.empty()) {
auto rv = unlink(m_configfn.c_str());
ASSERT_EQ(rv, 0) << "Failed to delete temp file ("
<< strerror(errno) << ", errno=" << errno << ")";
}
}

virtual std::string GetConfig() = 0;

std::string m_configfn;
std::unique_ptr<XrdSysLogger> m_log;
};

class FileSystemS3VirtualBucket : public FileSystemFixtureBase {
protected:
virtual std::string GetConfig() override {
return R"(
s3.begin
s3.path_name /test
s3.bucket_name genome-browser
s3.service_name s3.amazonaws.com
s3.region us-east-1
s3.service_url https://s3.us-east-1.amazonaws.com
s3.url_style virtual
s3.end
)";
}
};

class FileSystemS3VirtualNoBucket : public FileSystemFixtureBase {
protected:
virtual std::string GetConfig() override {
return R"(
s3.begin
s3.path_name /test
s3.service_name s3.amazonaws.com
s3.region us-east-1
s3.service_url https://s3.us-east-1.amazonaws.com
s3.url_style virtual
s3.end
)";
}
};

class FileSystemS3PathBucket : public FileSystemFixtureBase {
protected:
virtual std::string GetConfig() override {
return R"(
s3.begin
s3.path_name /test
s3.service_name s3.amazonaws.com
s3.region us-east-1
s3.bucket_name genome-browser
s3.service_url https://s3.us-east-1.amazonaws.com
s3.url_style path
s3.end
)";
}
};

class FileSystemS3PathNoBucket : public FileSystemFixtureBase {
protected:
virtual std::string GetConfig() override {
return R"(
s3.begin
s3.path_name /test
s3.service_name s3.amazonaws.com
s3.region us-east-1
s3.service_url https://s3.us-east-1.amazonaws.com
s3.url_style path
s3.end
)";
}
};

// Regression test for when the service_url ends in a `/`
class FileSystemS3PathBucketSlash : public FileSystemFixtureBase {
protected:
virtual std::string GetConfig() override {
return R"(
s3.begin
s3.path_name /test
s3.service_name s3.amazonaws.com
s3.region us-east-1
s3.bucket_name genome-browser
s3.service_url https://s3.us-east-1.amazonaws.com/
s3.url_style path
s3.end
)";
}
};

TEST_F(FileSystemS3VirtualBucket, Create) {
EXPECT_NO_THROW(
{ S3FileSystem fs(m_log.get(), m_configfn.c_str(), nullptr); });
}

TEST_F(FileSystemS3VirtualNoBucket, Create) {
EXPECT_NO_THROW(
{ S3FileSystem fs(m_log.get(), m_configfn.c_str(), nullptr); });
}

TEST_F(FileSystemS3PathBucket, Create) {
EXPECT_NO_THROW(
{ S3FileSystem fs(m_log.get(), m_configfn.c_str(), nullptr); });
}

TEST_F(FileSystemS3PathNoBucket, Create) {
EXPECT_NO_THROW(
{ S3FileSystem fs(m_log.get(), m_configfn.c_str(), nullptr); });
}

TEST_F(FileSystemS3PathBucketSlash, Create) {
EXPECT_NO_THROW(
{ S3FileSystem fs(m_log.get(), m_configfn.c_str(), nullptr); });
}

// Tests where we query S3 test fixture
class FileSystemS3Fixture : public FileSystemFixtureBase {
void SetUp() override {
Expand Down

0 comments on commit 7e160ee

Please sign in to comment.