Skip to content
This repository was archived by the owner on Sep 27, 2019. It is now read-only.

Static Code Checks for Test Files #1257

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions script/validators/source_validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@
"src/codegen/util/cc_hash_table.cpp"
]

TEST_CASE_PATT = re.compile(r'TEST_F\(([a-zA-Z]+), ([a-zA-Z]+)\)')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the right place to define the regex?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so.


## ==============================================
## UTILITY FUNCTION DEFINITIONS
## ==============================================
Expand Down Expand Up @@ -202,6 +204,39 @@ def check_includes(file_path):
return file_status


def check_tests(file_path):
"""Checks test source files for correct class and method naming."""
# check class naming
class_status = True # For reporting class names.
test_status = True # For reporting test cases.
line_num = 0
with open(file_path, "r") as file:
for line in file:
line_num += 1
if line.startswith('class '):
class_name = line.split(' ')[1]
if not class_name.endswith('Tests'):
if class_status:
LOG.info("Invalid class name in %s", file_path)
class_status = False
LOG.info("Line %s: %s", line_num, line.strip())

else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should there be a separate loop for this? That will prevent interleaved output.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with this, we hope not to have thousands of those errors.

# Check test case names.
case_found = TEST_CASE_PATT.match(line)
if case_found and not case_found.groups()[1].endswith('Test'):
if test_status:
LOG.info("Invalid test name in %s", file_path)
test_status = False
LOG.info("Line %s: %s", line_num, line.strip())

if not class_status:
LOG.info("Test class names should end with 'Tests' suffix.")
if not test_status:
LOG.info("Test case names should end with 'Test' suffix.")

return class_status and test_status

VALIDATORS = [
check_common_patterns,
check_includes,
Expand All @@ -224,6 +259,11 @@ def validate_file(file_path):
if not validator(file_path):
file_status = False

relative_path = os.path.relpath(file_path, start=PELOTON_DIR)
if relative_path.startswith('/test/') and relative_path.endswith('.cpp'):
if not relative_path.endswith('_util.cpp'):
file_status = check_tests(file_path)

return file_status


Expand Down
38 changes: 19 additions & 19 deletions test/binder/binder_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,17 @@ using std::vector;
namespace peloton {
namespace test {

class BinderCorrectnessTest : public PelotonTest {
class BinderCorrectnessTests : public PelotonTests {
virtual void SetUp() override {
PelotonTest::SetUp();
PelotonTests::SetUp();
catalog::Catalog::GetInstance();
// NOTE: Catalog::GetInstance()->Bootstrap(), you can only call it once!
TestingExecutorUtil::InitializeDatabase(DEFAULT_DB_NAME);
}

virtual void TearDown() override {
TestingExecutorUtil::DeleteDatabase(DEFAULT_DB_NAME);
PelotonTest::TearDown();
PelotonTests::TearDown();
}
};

Expand Down Expand Up @@ -101,7 +101,7 @@ void SetupTables(std::string database_name) {
}
}

TEST_F(BinderCorrectnessTest, SelectStatementTest) {
TEST_F(BinderCorrectnessTests, SelectStatementTest) {
std::string default_database_name = "test_db";
SetupTables(default_database_name);
auto &parser = parser::PostgresParser::GetInstance();
Expand All @@ -127,14 +127,14 @@ TEST_F(BinderCorrectnessTest, SelectStatementTest) {

oid_t db_oid =
catalog_ptr->GetDatabaseWithName(default_database_name, txn)->GetOid();
oid_t tableA_oid = catalog_ptr
->GetTableWithName(default_database_name,
DEFAULT_SCHEMA_NAME, "a", txn)
->GetOid();
oid_t tableB_oid = catalog_ptr
->GetTableWithName(default_database_name,
DEFAULT_SCHEMA_NAME, "b", txn)
->GetOid();
oid_t tableA_oid =
catalog_ptr->GetTableWithName(default_database_name, DEFAULT_SCHEMA_NAME,
"a", txn)
->GetOid();
oid_t tableB_oid =
catalog_ptr->GetTableWithName(default_database_name, DEFAULT_SCHEMA_NAME,
"b", txn)
->GetOid();
txn_manager.CommitTransaction(txn);

// Check select_list
Expand Down Expand Up @@ -250,7 +250,7 @@ TEST_F(BinderCorrectnessTest, SelectStatementTest) {
// instead of TupleValueExpression to represent column. We can only add this
// test after UpdateStatement is changed

TEST_F(BinderCorrectnessTest, DeleteStatementTest) {
TEST_F(BinderCorrectnessTests, DeleteStatementTest) {
std::string default_database_name = "test_db";
SetupTables(default_database_name);
auto &parser = parser::PostgresParser::GetInstance();
Expand All @@ -260,10 +260,10 @@ TEST_F(BinderCorrectnessTest, DeleteStatementTest) {
auto txn = txn_manager.BeginTransaction();
oid_t db_oid =
catalog_ptr->GetDatabaseWithName(default_database_name, txn)->GetOid();
oid_t tableB_oid = catalog_ptr
->GetTableWithName(default_database_name,
DEFAULT_SCHEMA_NAME, "b", txn)
->GetOid();
oid_t tableB_oid =
catalog_ptr->GetTableWithName(default_database_name, DEFAULT_SCHEMA_NAME,
"b", txn)
->GetOid();

string deleteSQL = "DELETE FROM b WHERE 1 = b1 AND b2 = 'str'";
unique_ptr<binder::BindNodeVisitor> binder(
Expand Down Expand Up @@ -292,7 +292,7 @@ TEST_F(BinderCorrectnessTest, DeleteStatementTest) {
txn_manager.CommitTransaction(txn);
}

TEST_F(BinderCorrectnessTest, BindDepthTest) {
TEST_F(BinderCorrectnessTests, BindDepthTest) {
std::string default_database_name = "test_db";
SetupTables(default_database_name);
auto &parser = parser::PostgresParser::GetInstance();
Expand Down Expand Up @@ -382,7 +382,7 @@ TEST_F(BinderCorrectnessTest, BindDepthTest) {
txn_manager.CommitTransaction(txn);
}

TEST_F(BinderCorrectnessTest, FunctionExpressionTest) {
TEST_F(BinderCorrectnessTests, FunctionExpressionTest) {
auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
auto txn = txn_manager.BeginTransaction();

Expand Down
2 changes: 1 addition & 1 deletion test/brain/query_clusterer_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ bool ClusterCorrectness(std::set<brain::Cluster *> clusters,
return check;
}

class QueryClustererTests : public PelotonTest {};
class QueryClustererTests : public PelotonTests {};

TEST_F(QueryClustererTests, ClusterTest) {
int num_features = 5;
Expand Down
2 changes: 1 addition & 1 deletion test/brain/query_logger_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
namespace peloton {
namespace test {

class QueryLoggerTests : public PelotonTest {
class QueryLoggerTests : public PelotonTests {
protected:
void SetUp() override {
settings::SettingsManager::SetBool(settings::SettingId::brain, true);
Expand Down
6 changes: 3 additions & 3 deletions test/brain/tensorflow_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace test {
// Tensorflow Tests
//===--------------------------------------------------------------------===//

class TensorflowTests : public PelotonTest {};
class TensorflowTests : public PelotonTests {};

TEST_F(TensorflowTests, BasicTFTest) {
// Check that the tensorflow library imports and prints version info correctly
Expand All @@ -46,8 +46,8 @@ TEST_F(TensorflowTests, SineWavePredictionTest) {
int NUM_WAVES = 3;
matrix_eig data = matrix_eig::Zero(NUM_SAMPLES, NUM_WAVES);
for (int i = 0; i < NUM_WAVES; i++) {
data.col(i).setLinSpaced(NUM_SAMPLES, NUM_SAMPLES * i,
NUM_SAMPLES * (i + 1) - 1);
data.col(i)
.setLinSpaced(NUM_SAMPLES, NUM_SAMPLES * i, NUM_SAMPLES * (i + 1) - 1);
data.col(i) = data.col(i).array().sin();
}

Expand Down
18 changes: 9 additions & 9 deletions test/catalog/catalog_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ namespace test {
// Catalog Tests
//===--------------------------------------------------------------------===//

class CatalogTests : public PelotonTest {};
class CatalogTests : public PelotonTests {};

TEST_F(CatalogTests, BootstrappingCatalog) {
TEST_F(CatalogTests, BootstrappingCatalogTest) {
auto catalog = catalog::Catalog::GetInstance();
catalog->Bootstrap();
EXPECT_EQ(1, storage::StorageManager::GetInstance()->GetDatabaseCount());
Expand All @@ -52,7 +52,7 @@ TEST_F(CatalogTests, BootstrappingCatalog) {
EXPECT_NE(nullptr, db_metric_table);
}
//
TEST_F(CatalogTests, CreatingDatabase) {
TEST_F(CatalogTests, CreatingDatabaseTest) {
auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
auto txn = txn_manager.BeginTransaction();
catalog::Catalog::GetInstance()->CreateDatabase("emp_db", txn);
Expand All @@ -62,7 +62,7 @@ TEST_F(CatalogTests, CreatingDatabase) {
txn_manager.CommitTransaction(txn);
}

TEST_F(CatalogTests, CreatingTable) {
TEST_F(CatalogTests, CreatingTableTest) {
auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
auto txn = txn_manager.BeginTransaction();
auto id_column = catalog::Column(
Expand Down Expand Up @@ -120,7 +120,7 @@ TEST_F(CatalogTests, CreatingTable) {
txn_manager.CommitTransaction(txn);
}

TEST_F(CatalogTests, TableObject) {
TEST_F(CatalogTests, TableObjectTest) {
auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
auto txn = txn_manager.BeginTransaction();

Expand Down Expand Up @@ -168,7 +168,7 @@ TEST_F(CatalogTests, TableObject) {
txn_manager.CommitTransaction(txn);
}

TEST_F(CatalogTests, TestingNamespace) {
TEST_F(CatalogTests, ingNamespaceTest) {
EXPECT_EQ(ResultType::SUCCESS, TestingSQLUtil::ExecuteSQLQuery("begin;"));
// create namespaces emp_ns0 and emp_ns1
EXPECT_EQ(ResultType::SUCCESS, TestingSQLUtil::ExecuteSQLQuery(
Expand Down Expand Up @@ -231,7 +231,7 @@ TEST_F(CatalogTests, TestingNamespace) {
EXPECT_EQ(ResultType::ABORTED, TestingSQLUtil::ExecuteSQLQuery("commit;"));
}

TEST_F(CatalogTests, DroppingTable) {
TEST_F(CatalogTests, DroppingTableTest) {
auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
auto txn = txn_manager.BeginTransaction();
auto catalog = catalog::Catalog::GetInstance();
Expand Down Expand Up @@ -293,7 +293,7 @@ TEST_F(CatalogTests, DroppingTable) {
txn_manager.CommitTransaction(txn);
}

TEST_F(CatalogTests, DroppingDatabase) {
TEST_F(CatalogTests, DroppingDatabaseTest) {
auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
auto txn = txn_manager.BeginTransaction();
catalog::Catalog::GetInstance()->DropDatabaseWithName("emp_db", txn);
Expand All @@ -304,7 +304,7 @@ TEST_F(CatalogTests, DroppingDatabase) {
txn_manager.CommitTransaction(txn);
}

TEST_F(CatalogTests, DroppingCatalog) {
TEST_F(CatalogTests, DroppingCatalogTest) {
auto catalog = catalog::Catalog::GetInstance();
EXPECT_NE(nullptr, catalog);
}
Expand Down
4 changes: 2 additions & 2 deletions test/catalog/constraints_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ namespace test {
// Constraints Tests
//===--------------------------------------------------------------------===//

class ConstraintsTests : public PelotonTest {};
class ConstraintsTests : public PelotonTests {};

#ifdef CONSTRAINT_NOTNULL_TEST
TEST_F(ConstraintsTests, NOTNULLTest) {
Expand Down Expand Up @@ -112,7 +112,7 @@ TEST_F(ConstraintsTests, NOTNULLTest) {
#endif

#ifdef CONSTRAINT_DEFAULT_TEST
TEST_F(ConstraintsTests, DEFAULTTEST) {
TEST_F(ConstraintsTests, DEFAULTTESTTest) {
// Set all of the columns to be NOT NULL
std::vector<std::vector<catalog::Constraint>> constraints;
for (int i = 0; i < CONSTRAINTS_NUM_COLS; i++) {
Expand Down
9 changes: 4 additions & 5 deletions test/catalog/manager_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
//
//===----------------------------------------------------------------------===//


#include "common/harness.h"

#include "catalog/manager.h"
Expand All @@ -27,10 +26,9 @@ namespace test {
// Manager Tests
//===--------------------------------------------------------------------===//

class ManagerTests : public PelotonTest {};
class ManagerTests : public PelotonTests {};

void AddTileGroup(UNUSED_ATTRIBUTE uint64_t thread_id) {

// TILES
std::vector<std::string> tile_column_names;
std::vector<std::vector<std::string>> column_names;
Expand All @@ -42,8 +40,9 @@ void AddTileGroup(UNUSED_ATTRIBUTE uint64_t thread_id) {
std::vector<catalog::Column> columns;

// SCHEMA
catalog::Column column1(type::TypeId::INTEGER, type::Type::GetTypeSize(type::TypeId::INTEGER),
"A", true);
catalog::Column column1(type::TypeId::INTEGER,
type::Type::GetTypeSize(type::TypeId::INTEGER), "A",
true);
columns.push_back(column1);

std::unique_ptr<catalog::Schema> schema1(new catalog::Schema(columns));
Expand Down
Loading