Skip to content

Commit

Permalink
[geometry] Meshcat's server returns 404 (not 200) on failures (RobotL…
Browse files Browse the repository at this point in the history
…ocomotion#20790)

This is also an opportunity to start to carve helper functions and
classes out into separate files for easier maintenance.

Two of the new functions are public and widely relevant:
ReadFile and ReadFileOrThrow.
  • Loading branch information
jwnimmer-tri authored Jan 18, 2024
1 parent 5a96aa5 commit 72ece57
Show file tree
Hide file tree
Showing 21 changed files with 248 additions and 171 deletions.
12 changes: 12 additions & 0 deletions bindings/pydrake/geometry/test/visualizers_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,18 @@ def test_meshcat(self):
# The pose is None because no meshcat session has broadcast its pose.
self.assertIsNone(meshcat.GetTrackedCameraPose())

def test_meshcat_404(self):
meshcat = mut.Meshcat()

good_url = meshcat.web_url()
with urllib.request.urlopen(good_url) as response:
self.assertTrue(response.read(1))

bad_url = f"{good_url}/no_such_file"
with self.assertRaisesRegex(Exception, "HTTP.*404"):
with urllib.request.urlopen(bad_url) as response:
response.read(1)

def test_meshcat_animation(self):
animation = mut.MeshcatAnimation(frames_per_second=64)
self.assertEqual(animation.frames_per_second(), 64)
Expand Down
1 change: 1 addition & 0 deletions common/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,7 @@ drake_cc_googletest(
":drake_path",
":find_resource",
"//common/test_utilities:expect_no_throw",
"//common/test_utilities:expect_throws_message",
],
)

Expand Down
24 changes: 23 additions & 1 deletion common/find_resource.cc
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#include "drake/common/find_resource.h"

#include <cstdlib>
#include <filesystem>
#include <fstream>
#include <sstream>
#include <stdexcept>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -291,4 +293,24 @@ string FindResourceOrThrow(const string& resource_path) {
return FindResource(resource_path).get_absolute_path_or_throw();
}

std::optional<string> ReadFile(const std::filesystem::path& path) {
std::optional<string> result;
std::ifstream input(path, std::ios::binary);
if (input.is_open()) {
std::stringstream content;
content << input.rdbuf();
result.emplace(std::move(content).str());
}
return result;
}

std::string ReadFileOrThrow(const std::filesystem::path& path) {
std::optional<string> result = ReadFile(path);
if (!result) {
throw std::runtime_error(
fmt::format("Error reading from '{}'", path.string()));
}
return std::move(*result);
}

} // namespace drake
9 changes: 9 additions & 0 deletions common/find_resource.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include <filesystem>
#include <optional>
#include <string>
#include <vector>
Expand Down Expand Up @@ -125,4 +126,12 @@ std::string FindResourceOrThrow(const std::string& resource_path);
/// copy of Drake, in case other methods have failed.
extern const char* const kDrakeResourceRootEnvironmentVariableName;

/// Returns the content of the file at the given path, or nullopt if it cannot
/// be read. Note that the path is a filesystem path, not a `resource_path`.
std::optional<std::string> ReadFile(const std::filesystem::path& path);

/// Returns the content of the file at the given path, or throws if it cannot
/// be read. Note that the path is a filesystem path, not a `resource_path`.
std::string ReadFileOrThrow(const std::filesystem::path& path);

} // namespace drake
18 changes: 9 additions & 9 deletions common/test/find_resource_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@

#include <cstdlib>
#include <filesystem>
#include <fstream>
#include <functional>
#include <memory>
#include <sstream>
#include <stdexcept>
#include <string>

Expand All @@ -16,6 +14,7 @@
#include "drake/common/drake_path.h"
#include "drake/common/drake_throw.h"
#include "drake/common/test_utilities/expect_no_throw.h"
#include "drake/common/test_utilities/expect_throws_message.h"

using std::string;

Expand Down Expand Up @@ -84,13 +83,8 @@ GTEST_TEST(FindResourceTest, FoundDeclaredData) {
// The path is the correct answer.
ASSERT_FALSE(absolute_path.empty());
EXPECT_EQ(absolute_path[0], '/');
std::ifstream input(absolute_path, std::ios::binary);
ASSERT_TRUE(input);
std::stringstream buffer;
buffer << input.rdbuf();
EXPECT_EQ(
buffer.str(),
"Test data for drake/common/test/find_resource_test.cc.\n");
EXPECT_EQ(ReadFileOrThrow(absolute_path),
"Test data for drake/common/test/find_resource_test.cc.\n");

// Sugar works the same way.
EXPECT_EQ(FindResourceOrThrow(relpath), absolute_path);
Expand Down Expand Up @@ -120,5 +114,11 @@ GTEST_TEST(GetDrakePathTest, PathIncludesDrake) {
EXPECT_TRUE(std::filesystem::exists(expected));
}

GTEST_TEST(ReadFileTest, NoSuchPath) {
EXPECT_FALSE(ReadFile("/foo/bar/missing"));
DRAKE_EXPECT_THROWS_MESSAGE(ReadFileOrThrow("/foo/bar/missing"),
"Error reading.*/foo/bar/missing.*");
}

} // namespace
} // namespace drake
12 changes: 1 addition & 11 deletions common/yaml/test/yaml_doxygen_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,6 @@ std::string WriteTemp(const std::string& data) {
return filename;
}

// Read data from a scratch file.
// (This is a test helper, not part of the Doxygen header.)
std::string ReadTemp(const std::string& filename) {
std::ifstream input(filename, std::ios::binary);
DRAKE_DEMAND(!input.fail());
std::stringstream buffer;
buffer << input.rdbuf();
return buffer.str();
}

// This is an example from the Doxygen header.
struct MyData {
template <typename Archive>
Expand Down Expand Up @@ -80,7 +70,7 @@ GTEST_TEST(YamlDoxygenTest, ExamplesSaving) {
SaveYamlFile(output_filename, data);

// This data is an example from the Doxygen header.
const std::string output = ReadTemp(output_filename);
const std::string output = ReadFileOrThrow(output_filename);
const std::string expected_output = R"""(
foo: 4.0
bar: [5.0, 6.0]
Expand Down
12 changes: 2 additions & 10 deletions common/yaml/test/yaml_io_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,6 @@ namespace yaml {
namespace test {
namespace {

std::string ReadTestFileAsString(const std::string& filename) {
std::ifstream input(filename, std::ios::binary);
DRAKE_DEMAND(!input.fail());
std::stringstream buffer;
buffer << input.rdbuf();
return buffer.str();
}

GTEST_TEST(YamlIoTest, LoadString) {
const std::string data = R"""(
value:
Expand Down Expand Up @@ -182,7 +174,7 @@ GTEST_TEST(YamlIoTest, SaveFile) {
const std::string filename = temp_directory() + "/SaveFile1.yaml";
const StringStruct data{.value = "save_file_1"};
SaveYamlFile(filename, data);
const auto result = ReadTestFileAsString(filename);
const auto result = ReadFileOrThrow(filename);
EXPECT_EQ(result, "value: save_file_1\n");
}

Expand All @@ -194,7 +186,7 @@ GTEST_TEST(YamlIoTest, SaveFileAllArgs) {
data.value.insert({"save_file_4", 1.0});
ASSERT_EQ(data.value.size(), 2);
SaveYamlFile(filename, data, child_name, {defaults});
const auto result = ReadTestFileAsString(filename);
const auto result = ReadFileOrThrow(filename);
EXPECT_EQ(result, "some_child:\n value:\n save_file_4: 1.0\n");
}

Expand Down
14 changes: 13 additions & 1 deletion geometry/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -452,13 +452,18 @@ drake_cc_googletest(

drake_cc_library(
name = "meshcat",
srcs = ["meshcat.cc"],
srcs = [
"meshcat.cc",
"meshcat_internal.cc",
],
hdrs = [
"meshcat.h",
"meshcat_internal.h",
"meshcat_types_internal.h",
],
data = [":meshcat_resources"],
install_hdrs_exclude = [
"meshcat_internal.h",
"meshcat_types_internal.h",
],
interface_deps = [
Expand Down Expand Up @@ -535,6 +540,13 @@ drake_cc_googletest(
],
)

drake_cc_googletest(
name = "meshcat_internal_test",
deps = [
":meshcat",
],
)

drake_cc_googletest(
name = "meshcat_denied_test",
allow_network = ["none"],
Expand Down
Loading

0 comments on commit 72ece57

Please sign in to comment.