Skip to content

Commit

Permalink
Merge pull request #417 from mmd-osm/patch/cleanup16
Browse files Browse the repository at this point in the history 
Various unit test linter fixes
  • Loading branch information
@mmd-osm
mmd-osm authored Jul 7, 2024
2 parents 641452b + ace09d9 commit d4d1d02
Show file tree
Hide file tree
Showing 10 changed files with 154 additions and 146 deletions.
51 changes: 28 additions & 23 deletions test/staticxml.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace {
struct bool_alpha {
bool data;
bool_alpha() = default;
bool_alpha(bool data) : data(data) {}
explicit bool_alpha(bool data) : data(data) {}
operator bool() const { return data; }
friend std::ostream &operator<<(std::ostream &out, bool_alpha b) {
out << std::boolalpha << b.data;
Expand All @@ -46,7 +46,8 @@ struct bool_alpha {

struct node {
element_info m_info;
double m_lon, m_lat;
double m_lon;
double m_lat;
tags_t m_tags;
};

Expand Down Expand Up @@ -78,8 +79,8 @@ struct database {
template <typename T>
std::optional<T> opt_attribute(std::string_view name, const xmlChar **attributes) {
while (*attributes != nullptr) {
auto name_attr = (const char *)(*attributes++);
std::string_view attr((const char *)name_attr);
auto* name_attr = (const char *)(*attributes++);
std::string_view attr(name_attr);
if (attr == name) {
return boost::lexical_cast<T>((const char *)(*attributes));
}
Expand Down Expand Up @@ -117,12 +118,12 @@ void parse_changeset_info(changeset_info &info, const xmlChar **attributes) {
info.display_name = opt_attribute<std::string>("user", attributes);
info.bounding_box = {};

std::optional<double> min_lat = opt_attribute<double>("min_lat", attributes);
std::optional<double> min_lon = opt_attribute<double>("min_lon", attributes);
std::optional<double> max_lat = opt_attribute<double>("max_lat", attributes);
std::optional<double> max_lon = opt_attribute<double>("max_lon", attributes);
auto min_lat = opt_attribute<double>("min_lat", attributes);
auto min_lon = opt_attribute<double>("min_lon", attributes);
auto max_lat = opt_attribute<double>("max_lat", attributes);
auto max_lon = opt_attribute<double>("max_lon", attributes);

if ((min_lat) && (min_lon) && (max_lat) && (max_lon)) {
if (min_lat && min_lon && max_lat && max_lon) {
info.bounding_box = bbox(*min_lat, *min_lon, *max_lat, *max_lon);
}

Expand All @@ -131,7 +132,7 @@ void parse_changeset_info(changeset_info &info, const xmlChar **attributes) {
}

struct xml_parser {
xml_parser(database *db)
explicit xml_parser(database *db)
: m_db(db) {}

static void start_element(void *ctx, const xmlChar *name_cstr,
Expand Down Expand Up @@ -314,10 +315,8 @@ inline void write_element<relation>(const relation &r, output_formatter &formatt
struct static_data_selection : public data_selection {
explicit static_data_selection(database& db) : static_data_selection(db, {}, {}) {}

explicit static_data_selection(database& db, user_roles_t m_user_roles, oauth2_tokens m_oauth2_tokens)
explicit static_data_selection(database& db, const user_roles_t& m_user_roles, const oauth2_tokens& m_oauth2_tokens)
: m_db(db)
, m_include_changeset_comments(false)
, m_redactions_visible(false)
, m_user_roles(m_user_roles)
, m_oauth2_tokens(m_oauth2_tokens){}

Expand Down Expand Up @@ -590,7 +589,7 @@ struct static_data_selection : public data_selection {

std::optional< osm_user_id_t > get_user_id_for_oauth2_token(
const std::string &token_id, bool &expired, bool &revoked,
bool &allow_api_write) {
bool &allow_api_write) override {

auto itr = m_oauth2_tokens.find(token_id);
if (itr != m_oauth2_tokens.end())
Expand Down Expand Up @@ -680,7 +679,7 @@ struct static_data_selection : public data_selection {

template <typename T>
int select(std::set<osm_nwr_id_t> &found_ids,
const std::vector<osm_nwr_id_t> select_ids) const {
const std::vector<osm_nwr_id_t>& select_ids) const {
int selected = 0;
for (osm_nwr_id_t id : select_ids) {
auto t = find_current<T>(id);
Expand Down Expand Up @@ -715,7 +714,8 @@ struct static_data_selection : public data_selection {
int selected = 0;
for (osm_nwr_id_t id : ids) {
using element_map_t = std::map<id_version, T>;
id_version idv_start(id, 0), idv_end(id+1, 0);
id_version idv_start(id, 0);
id_version idv_end(id+1, 0);
const element_map_t &m = map_of<T>();
if (!m.empty()) {
auto itr = m.lower_bound(idv_start);
Expand Down Expand Up @@ -757,9 +757,14 @@ struct static_data_selection : public data_selection {

database& m_db;
std::set<osm_changeset_id_t> m_changesets;
std::set<osm_nwr_id_t> m_nodes, m_ways, m_relations;
std::set<osm_edition_t> m_historic_nodes, m_historic_ways, m_historic_relations;
bool m_include_changeset_comments, m_redactions_visible;
std::set<osm_nwr_id_t> m_nodes;
std::set<osm_nwr_id_t> m_ways;
std::set<osm_nwr_id_t> m_relations;
std::set<osm_edition_t> m_historic_nodes;
std::set<osm_edition_t> m_historic_ways;
std::set<osm_edition_t> m_historic_relations;
bool m_include_changeset_comments = false;
bool m_redactions_visible = false;
user_roles_t m_user_roles;
oauth2_tokens m_oauth2_tokens;
};
Expand All @@ -782,7 +787,7 @@ const std::map<id_version, relation> &static_data_selection::map_of<relation>()
struct factory : public data_selection::factory {
explicit factory(const std::string &file) : factory(file, {}, {}) {}

explicit factory(const std::string &file, user_roles_t user_roles, oauth2_tokens oauth2_tokens)
explicit factory(const std::string &file, const user_roles_t& user_roles, const oauth2_tokens& oauth2_tokens)
: m_database(parse_xml(file.c_str()))
, m_user_roles(user_roles)
, m_oauth2_tokens(oauth2_tokens) {}
Expand All @@ -806,11 +811,11 @@ struct factory : public data_selection::factory {
struct staticxml_backend : public backend {
staticxml_backend() : staticxml_backend(user_roles_t{}, oauth2_tokens{}) {}

staticxml_backend(user_roles_t user_roles, oauth2_tokens oauth2_tokens) {
staticxml_backend(const user_roles_t& user_roles, const oauth2_tokens& oauth2_tokens) :
m_user_roles(user_roles), m_oauth2_tokens(oauth2_tokens)
{
m_options.add_options()("file", po::value<std::string>()->required(),
"file to load static OSM XML from.");
m_user_roles = user_roles;
m_oauth2_tokens = oauth2_tokens;
}

~staticxml_backend() override = default;
Expand Down
49 changes: 26 additions & 23 deletions test/test_apidb_backend_changeset_uploads.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ class global_setting_enable_bbox_size_limiter_test_class : public global_setting

std::unique_ptr<xmlDoc, void (*)(xmlDoc *)> getDocument(const std::string &document)
{
return {xmlReadDoc((xmlChar *)(document.c_str()), NULL, NULL, XML_PARSE_PEDANTIC | XML_PARSE_NONET), xmlFreeDoc};
return {xmlReadDoc((xmlChar *)(document.c_str()), nullptr, nullptr, XML_PARSE_PEDANTIC | XML_PARSE_NONET), xmlFreeDoc};
}

std::optional<std::string> getXPath(xmlDoc* doc, std::string xpath)
std::optional<std::string> getXPath(xmlDoc* doc, const std::string& xpath)
{
std::unique_ptr< xmlXPathContext, void (*)(xmlXPathContextPtr) > xpathCtx =
{ xmlXPathNewContext(doc), xmlXPathFreeContext };
Expand Down Expand Up @@ -123,7 +123,7 @@ struct CGImapListener : Catch::TestEventListenerBase, DatabaseTestsFixture {
CATCH_REGISTER_LISTENER( CGImapListener )


std::string get_compressed_payload(const std::string &payload)
std::string get_compressed_payload(std::string_view payload)
{
std::stringstream body;
std::stringstream output;
Expand Down Expand Up @@ -322,7 +322,8 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_nodes", "[changeset][upload
if (lat > maxlat) maxlat = lat;
if (lon > maxlon) maxlon = lon;

node_updater->modify_node(lat, lon, 1, node_id, node_version++, {{"key", "value" + std::to_string(i)}});
node_updater->modify_node(lat, lon, 1, node_id, node_version, {{"key", "value" + std::to_string(i)}});
node_version++;
}
node_updater->process_modify_nodes();
auto bbox = node_updater->bbox();
Expand Down Expand Up @@ -359,10 +360,12 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_nodes", "[changeset][upload
auto upd = tdb.get_data_update();
auto node_updater = upd->get_node_updater(ctx, change_tracking);

node_updater->delete_node(1, node_id, node_version++, false);
node_updater->delete_node(1, node_id, node_version, false);
node_updater->process_delete_nodes();
upd->commit();

node_version++;

REQUIRE(change_tracking.deleted_node_ids.size() == 1);
REQUIRE(change_tracking.deleted_node_ids[0] == static_cast<osm_nwr_signed_id_t>(node_id));

Expand Down Expand Up @@ -493,7 +496,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_ways", "[changeset][upload]
way_id = change_tracking.created_way_ids[0].new_id;
way_version = change_tracking.created_way_ids[0].new_version;

for (const auto id : change_tracking.created_node_ids) {
for (const auto& id : change_tracking.created_node_ids) {
node_new_ids[-1 * id.old_id - 1] = id.new_id;
}

Expand Down Expand Up @@ -673,7 +676,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_ways", "[changeset][upload]
auto upd = tdb.get_data_update();
auto way_updater = upd->get_way_updater(ctx, change_tracking);

way_updater->modify_way(1, way_id, 666, {static_cast<osm_nwr_signed_id_t>(5934531745)}, {});
way_updater->modify_way(1, way_id, 666, {5934531745}, {});
REQUIRE_THROWS_MATCHES(way_updater->process_modify_ways(), http::conflict,
Catch::Message("Version mismatch: Provided 666, server had: 2 of Way 1"));
}
Expand Down Expand Up @@ -747,10 +750,12 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_ways", "[changeset][upload]
auto upd = tdb.get_data_update();
auto way_updater = upd->get_way_updater(ctx, change_tracking);

way_updater->delete_way(1, way_id, way_version++, false);
way_updater->delete_way(1, way_id, way_version, false);
way_updater->process_delete_ways();
upd->commit();

way_version++;

REQUIRE(change_tracking.deleted_way_ids.size() == 1);
REQUIRE(change_tracking.deleted_way_ids[0] == static_cast<osm_nwr_signed_id_t>(way_id));
{
Expand Down Expand Up @@ -879,7 +884,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_relations", "[changeset][up
way_updater->process_new_ways();

// Remember new_ids for later tests. old_ids -1, -2, -3 are mapped to 0, 1, 2
for (const auto id : change_tracking.created_node_ids) {
for (const auto& id : change_tracking.created_node_ids) {
node_new_ids[-1 * id.old_id - 1] = id.new_id;
}

Expand Down Expand Up @@ -996,7 +1001,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_relations", "[changeset][up

std::array<osm_nwr_id_t,2> n_new_ids;

for (const auto id : change_tracking.created_node_ids) {
for (const auto& id : change_tracking.created_node_ids) {
n_new_ids[-1 * id.old_id - 1] = id.new_id;
}

Expand Down Expand Up @@ -1262,7 +1267,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_relations", "[changeset][up
auto upd = tdb.get_data_update();
auto rel_updater = upd->get_relation_updater(ctx, change_tracking);

rel_updater->modify_relation(1, relation_id, 666, { {"Node", static_cast<osm_nwr_signed_id_t>(1434253485634), ""} }, {});
rel_updater->modify_relation(1, relation_id, 666, { {"Node", 1434253485634, ""} }, {});
REQUIRE_THROWS_MATCHES(rel_updater->process_modify_relations(), http::conflict,
Catch::Message("Version mismatch: Provided 666, server had: 2 of Relation 1"));
}
Expand Down Expand Up @@ -1457,10 +1462,12 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_relations", "[changeset][up
auto upd = tdb.get_data_update();
auto rel_updater = upd->get_relation_updater(ctx, change_tracking);

rel_updater->delete_relation(1, relation_id, relation_version++, false);
rel_updater->delete_relation(1, relation_id, relation_version, false);
rel_updater->process_delete_relations();
upd->commit();

relation_version++;

REQUIRE(change_tracking.deleted_relation_ids.size() == 1);
REQUIRE(change_tracking.deleted_relation_ids[0] == static_cast<osm_nwr_signed_id_t>(relation_id));

Expand Down Expand Up @@ -1653,7 +1660,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_relations", "[changeset][up
way_updater->process_new_ways();

// Remember new_ids for later tests. old_ids -1, -2, -3 are mapped to 0, 1, 2
for (const auto id : change_tracking.created_node_ids) {
for (const auto& id : change_tracking.created_node_ids) {
node_new_ids[-1 * id.old_id - 1] = id.new_id;
}

Expand Down Expand Up @@ -1734,10 +1741,10 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_relations", "[changeset][up
auto txn_2nd = factory->get_default_transaction();
auto upd_2nd = factory->make_data_update(*txn_2nd);

auto rel_updater = upd_2nd->get_relation_updater(ctx4, change_tracking_2nd);
rel_updater->delete_relation(2, static_cast<osm_nwr_signed_id_t>(relation_id), 1, false);
auto rel_updater2 = upd_2nd->get_relation_updater(ctx4, change_tracking_2nd);
rel_updater2->delete_relation(2, static_cast<osm_nwr_signed_id_t>(relation_id), 1, false);
// throws precondition_failed exception once the main process commits and releases the lock.
rel_updater->process_delete_relations();
rel_updater2->process_delete_relations();
upd_2nd->commit(); // not reached
});

Expand Down Expand Up @@ -1777,7 +1784,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_single_relations", "[changeset][up
}


std::vector<api06::diffresult_t> process_payload(test_database &tdb, osm_changeset_id_t changeset, osm_user_id_t uid, std::string payload)
std::vector<api06::diffresult_t> process_payload(test_database &tdb, osm_changeset_id_t changeset, osm_user_id_t uid, const std::string& payload)
{
auto sel = tdb.get_data_selection();
auto upd = tdb.get_data_update();
Expand Down Expand Up @@ -2445,9 +2452,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_osmchange_end_to_end", "[changeset
TEST_CASE_METHOD( DatabaseTestsFixture, "test_osmchange_rate_limiter", "[changeset][upload][db]" ) {

// Upload rate limiter enabling
auto test_settings = std::unique_ptr<
global_settings_enable_upload_rate_limiter_test_class >(
new global_settings_enable_upload_rate_limiter_test_class());
auto test_settings = std::make_unique< global_settings_enable_upload_rate_limiter_test_class >();
global_settings::set_configuration(std::move(test_settings));

const std::string bearertoken = "Bearer 4f41f2328befed5a33bcabdf14483081c8df996cbafc41e313417776e8fafae8";
Expand Down Expand Up @@ -2603,9 +2608,7 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_osmchange_rate_limiter", "[changes
TEST_CASE_METHOD( DatabaseTestsFixture, "test_osmchange_bbox_size_limiter", "[changeset][upload][db]" ) {

// Upload bbox size limiter enabling
auto test_settings = std::unique_ptr<
global_setting_enable_bbox_size_limiter_test_class >(
new global_setting_enable_bbox_size_limiter_test_class());
auto test_settings = std::make_unique< global_setting_enable_bbox_size_limiter_test_class >();
global_settings::set_configuration(std::move(test_settings));

const std::string bearertoken = "Bearer 4f41f2328befed5a33bcabdf14483081c8df996cbafc41e313417776e8fafae8";
Expand Down
30 changes: 14 additions & 16 deletions test/test_apidb_backend_changesets.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_changeset_with_tags", "[changeset]
REQUIRE(f.m_changesets.size() == 1);

tags_t tags;
tags.push_back(std::make_pair("test_key", "test_value"));
tags.push_back(std::make_pair("test_key2", "test_value2"));
tags.emplace_back("test_key", "test_value");
tags.emplace_back("test_key2", "test_value2");
REQUIRE(
f.m_changesets.front() ==
test_formatter::changeset_t(
Expand Down Expand Up @@ -356,13 +356,6 @@ void init_changesets(test_database &tdb) {

// Prepare users, changesets

// Note: previously used credentials for user id 31:
//
// pass_crypt: '3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg='
// pass_salt: 'sha512!10000!OUQLgtM7eD8huvanFT5/WtWaCwdOdrir8QOtFwxhO0A='
//
// Those are still being used in test_apidb_backend_changeset_uploads.cpp

tdb.run_sql(R"(
INSERT INTO users (id, email, pass_crypt, pass_salt, creation_time, display_name, data_public, status)
VALUES
Expand Down Expand Up @@ -566,8 +559,8 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_changeset_create", "[changeset][db
REQUIRE(f.m_changesets.size() == 1);

tags_t tags;
tags.push_back(std::make_pair("comment", "Just adding some streetnames"));
tags.push_back(std::make_pair("created_by", "JOSM 1.61"));
tags.emplace_back("comment", "Just adding some streetnames");
tags.emplace_back("created_by", "JOSM 1.61");
REQUIRE(
f.m_changesets.front() ==
test_formatter::changeset_t(
Expand All @@ -586,9 +579,14 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_changeset_create", "[changeset][db
comments_t(),
t));

// TODO: check users changeset count
// TODO: check changesets_subscribers table

// User 31 should have 1 changeset in total
auto validate_cs_count = tdb.run_sql("SELECT * FROM users where id = 31 and changesets_count = 1");
REQUIRE(validate_cs_count == 1); // found 1 matching row

// Also user 31 should be subscribed to changeset 500
auto validate_cs_subscribers = tdb.run_sql("SELECT * FROM changesets_subscribers where subscriber_id = 31 and changeset_id = 500");
REQUIRE(validate_cs_subscribers == 1); // found 1 matching row
}

}
Expand Down Expand Up @@ -796,9 +794,9 @@ TEST_CASE_METHOD( DatabaseTestsFixture, "test_changeset_update", "[changeset][db
REQUIRE(f.m_changesets.size() == 1); // should have written one changeset 52.

tags_t tags;
tags.push_back(std::make_pair("tag1", "value1"));
tags.push_back(std::make_pair("tag2", "value2"));
tags.push_back(std::make_pair("tag3", "value3"));
tags.emplace_back("tag1", "value1");
tags.emplace_back("tag2", "value2");
tags.emplace_back("tag3", "value3");

REQUIRE(
f.m_changesets.front() ==
Expand Down
Loading

0 comments on commit d4d1d02

Please sign in to comment.