properly use defined source and build dirs for testing #4392

kevinkreiser · 2023-11-14T13:24:51Z

trivial one but in a lot of the tests we dont use directories in a CWD safe way which causes some tests to fail when you run them outside of their narrow working criteria. this pr simply fixes the tests that rely on paths to either source stuff or stuff we are building in the build directory

nilsnolde · 2023-11-14T13:27:16Z

ah thanks for the chore! never had the motivation so far, but it's so annoying to get all the test maps dumped in the wrong directory and messing up git (plus the inability to run them in some ways..)

kevinkreiser · 2023-11-14T14:07:23Z

yeah was working on #4390 and it was annoying me so last night i decided i was going to remedy it. i didnt check the gurka directory yet but i can get that in another pass

nilsnolde · 2023-11-14T14:47:22Z

Should we just add the VALHALLA_BUILD_DIR to test::make_config and be done with it? That'd give us all gurka tests for free and it seems there's only very few tests calling make_config with the right path so far, should be way less changes?

kevinkreiser · 2023-11-14T15:22:10Z

i had the same th ought yep

…be there

kevinkreiser · 2023-11-23T02:38:12Z

test/CMakeLists.txt

+target_compile_definitions(valhalla_test PRIVATE
+  VALHALLA_SOURCE_DIR="${VALHALLA_SOURCE_DIR}/"
+  VALHALLA_BUILD_DIR="${VALHALLA_BUILD_DIR}/")


all test targets get these two dirs defined so that..

kevinkreiser · 2023-11-23T02:38:41Z

test/actor.cc

-#if !defined(VALHALLA_SOURCE_DIR)
-#define VALHALLA_SOURCE_DIR
-#endif
-


we dont have to have this random hack in there and we always get the right value (i have no idea why this kind of thing was hacked in here..)

kevinkreiser · 2023-11-23T02:39:21Z

test/countryaccess.cc

 using namespace valhalla::mjolnir;
 using namespace valhalla::baldr;

 namespace {

-const std::string config_file = "test/test_config_country";
+const std::string config_file = VALHALLA_BUILD_DIR "test/test_config_country";


in places where we dont use make_config we use the thing manually

kevinkreiser · 2023-11-23T02:39:48Z

test/gurka/test_gtfs.cc

@@ -151,16 +151,16 @@ const gurka::ways ways = {{"AB", {{"highway", "primary"}}}, {"BC", {{"highway",

 boost::property_tree::ptree get_config() {

-  return test::make_config(VALHALLA_BUILD_DIR "test/data/transit_tests",
+  return test::make_config("test/data/transit_tests",


in places where we do use make_config we rely on it to do the work for us

kevinkreiser · 2023-11-23T02:42:16Z

test/gurka/test_match.cc

@@ -334,7 +335,7 @@ TEST(MapMatchRoute, IgnoreRestrictions) {
                                       },
                                       {{"type", "restriction"}, {"restriction", "no_straight_on"}}}};
  const gurka::map map =
-      gurka::buildtiles(layout, ways, {}, relations, "test/data/mapmatch_restrictions",
+      gurka::buildtiles(layout, ways, {}, relations, VALHALLA_BUILD_DIR "test/data/mapmatch_restrictions",
                        {{"mjolnir.timezone", VALHALLA_BUILD_DIR "test/data/tz.sqlite"}});


i felt it made sense to do it manually in these cases where we use the config option overrides because if we rely on make_config doing the right thing by prefixing directories with VALHALLA_BUILD_DIR, but then we add a config override for timezone and we ourselves dont prefix it, then we've defeated ourselves.

alternatively i could have written code to see if any override value begins with "test/data" and then prefixed that but it felt a little too much? i dont feel stronly about it

not sure I understand this: on line 205 you're removing the prefix for tzdb, on line 337 you're adding it for the graph build (which should be covered already by make_config) and on line 339 you left it again for tzdb. probably an oversight?

IMO it'd be more consistent that noone ever has to specify VALHALLA_BUILD_DIR manually, but it's also a much rarer thing to do, so I'm ok with it too.

kevinkreiser · 2023-11-23T02:45:48Z

test/test.cc

+  // we should use absolute paths so the binaries work regardless of CWD
+  std::string absolute;
+  if (path_prefix.find("test/") == 0) {
+    absolute = VALHALLA_BUILD_DIR;
+  }
+
  // force the paths to be different
-  boost::replace_all(defaults, "%%", path_prefix);
+  boost::replace_all(defaults, "%%", absolute + path_prefix);


this is the important part, we detect if your data prefix doesnt have the absolute path on and add it if not. this lets the binaries be run from any working directory (which can be useful)

also note that gurka::buildtiles calls into this function so its covered too

…_dir

kevinkreiser · 2023-11-23T02:47:23Z

this is ready for review

nilsnolde · 2023-11-23T08:03:23Z

test/gurka/test_gtfs.cc

                            {"mjolnir.transit_pbf_limit",
                             "1"}, // so we create more than one file per tile
                            {"mjolnir.hierarchy", "1"},
-                            {"mjolnir.timezone", VALHALLA_BUILD_DIR "test/data/tz.sqlite"},


hm, do we really want to remove it here? or maybe it's added somewhere else I didn't yet?

yeah this is the primary open issue at this point. the signature for make_config takes first the directory argument and second a map of key values replacements, some of which might be things that are directories some not.

it was easy for me to patch the first arg when it didnt have the absolute path because i knew it was a directory but for the key values i dont know which ones are and arent directories. this means detecting and patching those is harder.

there are two options to do it automatically:

something simple where i see the value begins with "test/data" this will not catch everythign but it should get most of everything

figure out which ones should be directories by looking at the default config for the path value that got substituted in everywhere and then patching only those that were substituted paths

i think ill take a try at the 2nd one because then everything can be completely automatic and i can remove any of these that exist

that'd be much appreciated, albeit quite a sizable mechanical chore

nilsnolde · 2023-11-23T08:24:46Z

test/gurka/test_probable_restrictions.cc

@@ -67,7 +67,8 @@ class ProbableRestrictions : public testing::TestWithParam<int> {
              {gurka::node_member, "I", "via"},
          },
          {{"type", "restriction"}, {"restriction:probable", "no_left_turn @ probability=0"}}}};
-    map = gurka::buildtiles(layout, ways, {}, relations, "test/data/gurka_time_probable_restrictions",
+    map = gurka::buildtiles(layout, ways, {}, relations,
+                            VALHALLA_BUILD_DIR "test/data/gurka_time_probable_restrictions",


yeah I'm confused.. isn't this the graph directory? don't we set that now in make_config()?

same in lots of other places in gurka

we do and I went through 2 rounds of this and missed this in the last round. it's surprisingly annoying to find all occurances at midnight after a long day at work hahaha

properly use defined source and build dirs for testing

74843c4

lint

04238de

kevinkreiser added 2 commits November 21, 2023 10:35

automatically inject full path on config building if detected to not …

784e83f

…be there

lint

1df963b

kevinkreiser mentioned this pull request Nov 22, 2023

dont build shortcuts across access_restrictions #4326

Merged

remove as many useless bits of code as possible

424bcac

kevinkreiser commented Nov 23, 2023

View reviewed changes

kevinkreiser added 2 commits November 22, 2023 21:46

this dir is superfluous

f5c6ceb

Merge remote-tracking branch 'origin/master' into kk_fix_test_dir_src…

df575e2

…_dir

lint

6d4eb53

nilsnolde reviewed Nov 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

properly use defined source and build dirs for testing #4392

properly use defined source and build dirs for testing #4392

kevinkreiser commented Nov 14, 2023

nilsnolde commented Nov 14, 2023

kevinkreiser commented Nov 14, 2023

nilsnolde commented Nov 14, 2023 •

edited

kevinkreiser commented Nov 14, 2023

kevinkreiser Nov 23, 2023

kevinkreiser Nov 23, 2023

kevinkreiser Nov 23, 2023

kevinkreiser Nov 23, 2023

kevinkreiser Nov 23, 2023

nilsnolde Nov 23, 2023

kevinkreiser Nov 23, 2023

kevinkreiser Nov 23, 2023

kevinkreiser commented Nov 23, 2023

nilsnolde Nov 23, 2023

kevinkreiser Nov 23, 2023

nilsnolde Nov 24, 2023

nilsnolde Nov 23, 2023

nilsnolde Nov 23, 2023

kevinkreiser Nov 23, 2023

properly use defined source and build dirs for testing #4392

Are you sure you want to change the base?

properly use defined source and build dirs for testing #4392

Conversation

kevinkreiser commented Nov 14, 2023

nilsnolde commented Nov 14, 2023

kevinkreiser commented Nov 14, 2023

nilsnolde commented Nov 14, 2023 • edited

kevinkreiser commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsnolde commented Nov 14, 2023 •

edited