Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: replace tmpnam use #3497

Closed
wants to merge 1 commit into from
Closed
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
13 changes: 3 additions & 10 deletions tests/acceptance-tests/server_configuration_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

#include "mir/options/option.h"

#include "mir/test/file_utils.h"

#include <gtest/gtest.h>
#include <gmock/gmock.h>

Expand All @@ -29,15 +31,6 @@ using namespace testing;

namespace
{
std::string create_temp_dir()
{
char temp_dir[] = "temp_dir_XXXXXX";
if (mkdtemp(temp_dir) == nullptr)
throw std::system_error(errno, std::system_category(), "Failed to create temp dir");

return temp_dir;
}

struct ServerConfigurationOptions : mir_test_framework::HeadlessTest
{
MOCK_METHOD(void, command_line_handler, (std::vector<std::string> const&));
Expand Down Expand Up @@ -102,7 +95,7 @@ struct ServerConfigurationOptions : mir_test_framework::HeadlessTest
remove(temp_dir.c_str());
}

std::string const temp_dir = create_temp_dir();
std::string const temp_dir = mir::test::create_temp_dir();
std::string const fake_xdg_config_home = temp_dir + "/fake_xdg_config_home";
std::string const fake_home = temp_dir + "/fake_home";
std::string const fake_home_config = temp_dir + "/fake_home/.config";
Expand Down
29 changes: 29 additions & 0 deletions tests/include/mir/test/file_utils.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright © Canonical Ltd.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 or 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#ifndef MIR_TEST_FILE_UTILS_H_
#define MIR_TEST_FILE_UTILS_H_

namespace mir
{
namespace test
{
std::string create_temp_file();
std::string create_temp_dir();
}
}

#endif // MIR_TEST_FILE_UTILS_H_
1 change: 1 addition & 0 deletions tests/mir_test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ add_library(mir-public-test OBJECT
display_config_matchers.cpp
event_factory.cpp
event_matchers.cpp
file_utils.cpp
pipe.cpp
popen.cpp
signal.cpp
Expand Down
44 changes: 44 additions & 0 deletions tests/mir_test/file_utils.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright © Canonical Ltd.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 or 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#include <system_error>
#include <unistd.h>

#include "mir/test/file_utils.h"

std::string mir::test::create_temp_file()
{
char temp_file[] = "/tmp/temp_file_XXXXXX";
if (int fd = mkstemp(temp_file) == -1)
{
throw std::system_error(errno, std::system_category(), "Failed to create temp file");
}
else
{
close(fd);
}

return temp_file;
}
Comment on lines +22 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this suffers basically all the problems that tmpnam has.

But it's only used in test code where the problems of tmpnam aren't really problems, so 🤷‍♀️

But it would be reasonably easy to replace the current uses of tmpname with mkstemp directly. So even more 🤷‍♀️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, this suffers basically all the problems that tmpnam has.

Yeah this is just about silencing the linker warning. But you're right, we can just use mkstemp directly - thanks for the reality check.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, this suffers basically all the problems that tmpnam has.

Yeah this is just about silencing the linker warning. But you're right, we can just use mkstemp directly - thanks for the reality check.

I concur


std::string mir::test::create_temp_dir()
{
char temp_dir[] = "/tmp/temp_dir_XXXXXX";
if (mkdtemp(temp_dir) == nullptr)
throw std::system_error(errno, std::system_category(), "Failed to create temp dir");

return temp_dir;
}
3 changes: 3 additions & 0 deletions tests/miral/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ mir_generate_protocol_wrapper(miral-test "org_kde_kwin_" server-decoration.xml)

set_source_files_properties(server_example_decoration.cpp PROPERTIES COMPILE_FLAGS "-I${CMAKE_CURRENT_BINARY_DIR}")

target_include_directories(miral-test
PRIVATE ${PROJECT_SOURCE_DIR}/tests/include)

target_link_libraries(miral-test
mir-test-assist
PkgConfig::WAYLAND_CLIENT
Expand Down
4 changes: 3 additions & 1 deletion tests/miral/external_client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#include <miral/external_client.h>
#include <miral/x11_support.h>

#include "mir/test/file_utils.h"

#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include <fstream>
Expand All @@ -43,7 +45,7 @@ struct ExternalClient : miral::TestServer
miral::X11Support x11_disabled_by_default{};
miral::X11Support x11_enabled_by_default{miral::X11Support{}.default_to_enabled()};

std::string const output = tmpnam(nullptr);
std::string const output = mir::test::create_temp_file();
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a mir::Fd from mkstmp; the external_client.launch lines would then redirect to the fd using >&$FD.


auto client_env_value(std::string const& key) const -> std::string
{
Expand Down
5 changes: 4 additions & 1 deletion tests/miral/static_display_config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "mir/logging/logger.h"

#include <mir/test/doubles/mock_display_configuration.h>
#include "mir/test/file_utils.h"

#include <gtest/gtest.h>
#include <gmock/gmock.h>
Expand Down Expand Up @@ -151,7 +152,9 @@ struct StaticDisplayConfig : Test

TEST_F(StaticDisplayConfig, nonexistent_config_file_is_no_error)
{
miral::StaticDisplayConfig{tmpnam(nullptr)};
auto filename = mir::test::create_temp_file();
unlink(filename.c_str());
Comment on lines +155 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be create_temp_dir plus an arbitrary filename.

miral::StaticDisplayConfig{filename};
}

TEST_F(StaticDisplayConfig, empty_config_input_causes_AbnormalExit)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "mir/test/doubles/mock_scene_session.h"
#include "mir/scene/surface_observer.h"
#include "mir/test/doubles/explicit_executor.h"
#include "mir/test/file_utils.h"
#include "mir_test_framework/open_wrapper.h"

#include <filesystem>
Expand Down Expand Up @@ -194,7 +195,7 @@ TEST_F(DesktopFileManager, can_resolve_from_valid_flatpak_info)
std::stringstream flatpak_info_path;
flatpak_info_path << "/proc/" << PID << "/root/.flatpak-info";
auto const flatpak_info = flatpak_info_path.str();
auto tmp_file_name = std::tmpnam(NULL);
auto tmp_file_name = mir::test::create_temp_file().c_str();
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be mkstmp; nothing really uses the filename here.

{
std::ofstream tmp_file;
tmp_file.open(tmp_file_name);
Expand Down Expand Up @@ -228,7 +229,7 @@ TEST_F(DesktopFileManager, app_id_will_not_resolve_from_flatpak_info_when_name_i
std::stringstream flatpak_info_path;
flatpak_info_path << "/proc/" << PID << "/root/.flatpak-info";
auto const flatpak_info = flatpak_info_path.str();
auto tmp_file_name = std::tmpnam(NULL);
auto tmp_file_name = mir::test::create_temp_file().c_str();
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

{
std::ofstream tmp_file;
tmp_file.open(tmp_file_name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include "src/server/frontend_wayland/foreign_toplevel_manager_v1.h"
#include "mir/test/doubles/stub_main_loop.h"
#include "mir/test/file_utils.h"
#include <fstream>

#include <gmock/gmock.h>
Expand All @@ -43,9 +44,7 @@ struct GDesktopFileCache : Test
static void SetUpTestSuite()
{
// Establish the temporary directory
std::filesystem::path tmp_dir_path {std::filesystem::temp_directory_path() /= std::tmpnam(nullptr)};
std::filesystem::create_directories(tmp_dir_path);
std::string desktop_file_directory_name = tmp_dir_path;
std::string desktop_file_directory_name = mir::test::create_temp_dir();
setenv("XDG_DATA_DIRS", desktop_file_directory_name.c_str(), 1);
applications_dir = desktop_file_directory_name + "/applications";
mkdir(applications_dir.c_str(), S_IRWXU);
Expand Down
Loading