[Tile-serving] [openstreetmap/osm2pgsql] Use catch framework for testing (#966)
Jochen Topf
notifications at github.com
Wed Nov 13 18:08:39 UTC 2019
joto commented on this pull request.
I didn't get through everything, but here is what I got so far.
> namespace cleanup {
-// RAII structure to remove a file upon destruction. doesn't create or do
-// anything to the file when it constructs.
-struct file {
- file(const std::string &filename);
- ~file();
+/**
+ * RAII structure to remove a file upon destruction.
+ *
+ * Per defualt will also make sure that the file does not exist
s/defualt/default/
> + * RAII structure to remove a file upon destruction.
+ *
+ * Per defualt will also make sure that the file does not exist
+ * when it is constructed.
+ */
+class file_t
+{
+public:
+ file_t(std::string const &filename, bool remove_on_construct = true)
+ : m_filename(filename)
+ {
+ if (remove_on_construct)
+ delete_file(false);
+ }
+
+ ~file_t() { delete_file(true); }
Destructors should be noexcept
> private:
- // name of the file to be deleted upon destruction
- std::string m_filename;
-};
+ void delete_file(bool warn)
Should be noexcept, because it is called from destructor.
> +namespace db {
+
+/**
+ * Convenience class around tempdb_t that offers functions for
+ * data import from file and strings.
+ */
+class import_t
+{
+public:
+ void run_import(options_t options, char const *data,
+ std::string const &fmt = "opl")
+ {
+ options.database_options = m_db.db_options();
+
+ // setup the middle
+ std::shared_ptr<middle_t> middle(new middle_ram_t(&options));
make_shared?
> +
+ osmdata.start();
+
+ test_parse_t parser(options.bbox, options.append, &osmdata);
+
+ parser.stream_buffer(data, fmt);
+
+ osmdata.stop();
+ }
+
+ void run_file(options_t options, char const *file = nullptr)
+ {
+ options.database_options = m_db.db_options();
+
+ // setup the middle
+ std::shared_ptr<middle_t> middle(new middle_ram_t(&options));
make_shared? More of the same below
>
-struct conn
- : public boost::noncopyable,
- public std::enable_shared_from_this<conn> {
+ ~result_t() { PQclear(m_result); }
Destructor should be noexcept. In fact all functions in this class could probably be noexcept.
Same issue below.
> - * Assert that the database has a certain table_name
- * \param[in] table_name Name of the table to check, optionally schema-qualified
- * \throws std::runtime_error if missing the table
- */
- void assert_has_table(const std::string &table_name);
+class tempdb_t
+{
+public:
+ tempdb_t()
+ {
+ conn_t conn("dbname=postgres");
+
+ m_db_name =
+ (boost::format("osm2pgsql-test-%1%-%2%") % getpid() % time(nullptr))
+ .str();
+ conn.exec(boost::format("DROP DATABASE IF EXISTS \"%1%\"") % m_db_name);
Use C++11 raw string literals? `R"SQL(DROP DATABASE IF EXISTS "%1%")SQL"`
Is a bit unfamiliar, but saves on escaping...
> @@ -0,0 +1,192 @@
+#include <string>
+#include <vector>
+
+#include "catch.hpp"
+
+#include "common-pg.hpp"
+#include "db-copy.hpp"
+
+using fmt = boost::format;
Used only twice in this file plus twice more in `test-output-gazetteer.cpp`. Either remove or maybe put into common include so `common-pg.hpp` can also use it, which uses `boost::format` a lot.
> + conn.exec(fmt("CREATE TABLE test_copy_mgr (id int8%1%%2%)") %
+ (cols.empty() ? "" : ",") % cols);
+
+ auto table = std::make_shared<db_target_descr_t>();
+ table->name = "test_copy_mgr";
+ table->id = "id";
+
+ return table;
+}
+
+template <typename... ARGS>
+void add_row(db_copy_mgr_t &mgr, std::shared_ptr<db_target_descr_t> t,
+ ARGS &&... args)
+{
+ mgr.new_line(t);
+ mgr.add_columns(args...);
`std::forward<ARGS>(args)...` ?
> + std::vector<std::pair<std::string, std::string>> values)
+{
+ mgr.new_line(t);
+
+ mgr.add_column(id);
+ mgr.new_hash();
+ for (auto const &v : values) {
+ mgr.add_hash_elem(v.first, v.second);
+ }
+ mgr.finish_hash();
+ mgr.finish_line();
+
+ mgr.sync();
+}
+
+static void check_row(std::vector<std::string> row)
`std::vector<...> const & row`
Alternatively use` std::initializer_list` because these strings in the argument are all literals.
> +{
+ mgr.new_line(t);
+ mgr.add_column(id);
+ mgr.new_array();
+ for (auto v : values) {
+ mgr.add_array_elem(v);
+ }
+ mgr.finish_array();
+ mgr.finish_line();
+
+ mgr.sync();
+}
+
+static void add_hash(db_copy_mgr_t &mgr, std::shared_ptr<db_target_descr_t> t,
+ int id,
+ std::vector<std::pair<std::string, std::string>> values)
`std::vector<...> const &values`
> +}
+
+template <typename... ARGS>
+void add_row(db_copy_mgr_t &mgr, std::shared_ptr<db_target_descr_t> t,
+ ARGS &&... args)
+{
+ mgr.new_line(t);
+ mgr.add_columns(args...);
+ mgr.finish_line();
+
+ mgr.sync();
+}
+
+template <typename T>
+void add_array(db_copy_mgr_t &mgr, std::shared_ptr<db_target_descr_t> t, int id,
+ std::vector<T> values)
`const std::vector<T> const &values`
> + result_t res = query(cmd);
+ REQUIRE(res.status() == PGRES_TUPLES_OK);
+ REQUIRE(res.num_tuples() == 1);
+
+ std::string str = res.get_value(0, 0);
+ return boost::lexical_cast<T>(str);
+ }
+
+ void assert_double(double expected, std::string const &cmd) const
+ {
+ REQUIRE(Approx(expected) == require_scalar<double>(cmd));
+ }
+
+ void assert_null(std::string const &cmd) const
+ {
+ REQUIRE(require_scalar<std::string>(cmd) == "");
Does this check for `NULL` or empty string?
> +#include <osmium/osm/crc_zlib.hpp>
+
+#include "middle-pgsql.hpp"
+#include "middle-ram.hpp"
+
+#include "common-options.hpp"
+#include "common-pg.hpp"
+
+static pg::tempdb_t db;
+
+namespace {
+
+/// Simple osmium buffer to store object with some convenience.
+struct test_buffer_t
+{
+ size_t add_node(osmid_t id, double lat, double lon)
Here order is `lat, lon`, below `_location(lon, lat)`. Could be confusing. Maybe use `osmium::Location` as parameter?
> + template <typename T>
+ T const &get(size_t pos) const
+ {
+ return buf.get<T>(pos);
+ }
+
+ template <typename T>
+ T &get(size_t pos)
+ {
+ return buf.get<T>(pos);
+ }
+
+ osmium::memory::Buffer buf{4096, osmium::memory::Buffer::auto_grow::yes};
+};
+
+void expect_location(osmium::Location loc, osmium::Node const &expected)
Do we need this? Because `Location` internally uses integers just comparing locations could be enough.
> + buffer.get<osmium::Way>(buffer.add_way(3, {1234})).nodes();
+
+ // get it back
+ REQUIRE(mid_q->nodes_get_list(&(nodes)) == nodes.size());
+ expect_location(nodes[0].location(), node);
+
+ // other nodes are not retrievable
+ auto &n2 =
+ buffer.get<osmium::Way>(buffer.add_way(3, {1, 2, 1235})).nodes();
+ REQUIRE(mid_q->nodes_get_list(&(n2)) == 0);
+ }
+
+ SECTION("Set and retrieve a single way")
+ {
+ osmid_t way_id = 1;
+ double lat = 12.3456789;
These can be `const`. Probably lots of other places in the code where `const` could be added.
> + mid->flush(osmium::item_type::relation);
+
+ // get it back
+ osmium::memory::Buffer outbuf{4096,
+ osmium::memory::Buffer::auto_grow::yes};
+
+ REQUIRE(mid_q->ways_get(way_id, outbuf));
+
+ auto &way = outbuf.get<osmium::Way>(0);
+
+ CHECK(way.id() == way_id);
+ REQUIRE(way.nodes().size() == nds.size());
+
+ REQUIRE(mid_q->nodes_get_list(&(way.nodes())) == nds.size());
+ for (osmid_t i = 1; i <= 10; ++i) {
+ CHECK(way.nodes()[(size_t)i - 1].ref() == i);
`static_cast<std::size_t>(i)`
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/osm2pgsql/pull/966#pullrequestreview-316345458
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/tile-serving/attachments/20191113/b83b33cb/attachment.html>
More information about the Tile-serving
mailing list