blender
/
blender
136
406
Fork 589
Code Issues 6.3k Pull Requests 765 Projects 19 Wiki Activity

BLI: add function for changing working directory #104525

Merged
Sybren A. Stüvel merged 11 commits from SonnyCampbell_Unity/blender:change-working-directory into main 2023-02-20 15:58:49 +01:00
Conversation 3 Commits 11 Files Changed 4 +19 -10
2 changed files with 19 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit e988efdbab - Show all commits

1
source/blender/blenlib/intern/storage.c
View File

@ -58,6 +58,7 @@
#include "BLI_utildefines.h"
#if !defined(__APPLE__)
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-16 12:47:41 +01:00
Review
Copy Link

I think it would be nice to have a comment here that explains that the Apple implementation lives in storage_apply.mm.

I think it would be nice to have a comment here that explains that the Apple implementation lives in `storage_apply.mm`.
/* The implementation for Apple lives in storage_apple.mm.*/
bool BLI_change_working_dir(const char *dir)
{
BLI_assert(BLI_thread_is_main());

28
source/blender/blenlib/tests/BLI_fileops_test.cc
View File

@ -8,6 +8,20 @@
namespace blender::tests {
class ChangeWorkingDirectoryTest : public testing::Test {
public:
std::string test_temp_dir;
void TearDown() override
{
if (!test_temp_dir.empty()) {
BLI_delete(test_temp_dir.c_str(), true, false);
}
BLI_threadapi_exit();
}
};
TEST(fileops, fstream_open_string_filename)
{
const std::string test_files_dir = blender::tests::flags_test_asset_dir();
@ -39,7 +53,7 @@ TEST(fileops, fstream_open_charptr_filename)
/* Reading the file not tested here. That's deferred to `std::fstream` anyway. */
}
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-16 12:51:22 +01:00
Review
Copy Link

As far as I'm concerned you can just call BLI_delete() unconditionally. It'll return a failure code, but that's ignored anyway. I'll leave it up to you to decide which one you feel is the best approach; leaving it as-is and just clicking on "resolve conversation" is fine too.

As far as I'm concerned you can just call `BLI_delete()` unconditionally. It'll return a failure code, but that's ignored anyway. I'll leave it up to you to decide which one you feel is the best approach; leaving it as-is and just clicking on "resolve conversation" is fine too.
Sonny Campbell commented 2023-02-17 11:11:32 +01:00
Review
Copy Link

Will leave this one as is, if I remove the if (BLI_exists(test_temp_dir.c_str())) check and the directory doesn't exist, it prints 40: Unable to remove directory in the test output when it runs with the verbose flag, which might be confusing and perceived as an issue by a dev later on.

Will leave this one as is, if I remove the ` if (BLI_exists(test_temp_dir.c_str()))` check and the directory doesn't exist, it prints `40: Unable to remove directory` in the test output when it runs with the verbose flag, which might be confusing and perceived as an issue by a dev later on.
👍 1
TEST(fileops, change_working_directory)
TEST_F(ChangeWorkingDirectoryTest, change_working_directory)
{
/* Must use because BLI_change_working_dir() checks that we are on the main thread. */
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 15:14:08 +01:00
Review
Copy Link

Add some logging to this and the following ASSERT calls. When the test fails, it should be clear why it failed. You can do something like:

ASSERT_FALSE(result) << "a chdir to a non-existent directory is expected to fail";
Add some logging to this and the following ASSERT calls. When the test fails, it should be clear why it failed. You can do something like: ```cpp ASSERT_FALSE(result) << "a chdir to a non-existent directory is expected to fail"; ```
BLI_threadapi_init();
@ -50,7 +64,7 @@ TEST(fileops, change_working_directory)
}
const std::string temp_file_name(std::tmpnam(nullptr));
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-16 12:52:53 +01:00
Review
Copy Link

This one ASSERT_TRUE() shouldn't indirectly test the result of BLI_dir_create_recursive(). Just use ASSERT_TRUE(BLI_dir_create_recursive(test_temp_dir.c_str());, that way this error message can remain straight-forward and only about the function under test.

This one `ASSERT_TRUE()` shouldn't indirectly test the result of `BLI_dir_create_recursive()`. Just use `ASSERT_TRUE(BLI_dir_create_recursive(test_temp_dir.c_str());`, that way this error message can remain straight-forward and only about the function under test.
const std::string test_temp_dir = temp_file_name + "_новый";
test_temp_dir = temp_file_name + "_новый";
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-10 15:14:30 +01:00
Review
Copy Link

This should be using ASSERT_EQ.

This should be using `ASSERT_EQ`.
if (BLI_exists(test_temp_dir.c_str())) {
BLI_delete(test_temp_dir.c_str(), true, false);
@ -61,8 +75,7 @@ TEST(fileops, change_working_directory)
BLI_dir_create_recursive(test_temp_dir.c_str());
result = BLI_change_working_dir(test_temp_dir.c_str());
ASSERT_TRUE(result)
ASSERT_TRUE(BLI_change_working_dir(test_temp_dir.c_str()))
<< "temporary directory should have been created and should succeed changing directory.";
char cwd[FILE_MAX];
SonnyCampbell_Unity marked this conversation as resolved
Sybren A. Stüvel commented 2023-02-16 12:54:09 +01:00
Review
Copy Link

This should be done in a teardown routine, so that the environment is always cleaned up when the test is done.

This should be done in a teardown routine, so that the environment is always cleaned up when the test is done.
@ -74,13 +87,8 @@ TEST(fileops, change_working_directory)
<< "the path of the current working directory should equal the path of the temporary "
"directory that was created.";
result = BLI_change_working_dir(original_wd);
ASSERT_TRUE(result)
ASSERT_TRUE(BLI_change_working_dir(original_wd))
<< "changing directory back to the original working directory should succeed.";
BLI_delete(test_temp_dir.c_str(), true, false);
BLI_threadapi_exit();
}
} // namespace blender::tests