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

Create an output directory for test files. #6

Merged
merged 1 commit into from
Apr 21, 2020
Merged

Conversation

joergi-w
Copy link
Member

@joergi-w joergi-w commented Apr 4, 2020

This PR adds an output directory for test files. In the CLI tests the path can be obtained with the workdir() function.

This PR is needed as a preliminary step for seqan/product_backlog#4.

@joergi-w joergi-w requested a review from marehr April 4, 2020 13:37
test/cli/cli_test.cpp Outdated Show resolved Hide resolved
return std::filesystem::path{test_name + filename};
}

// Connect the test directory with the filename.
Copy link
Member

Choose a reason for hiding this comment

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

Connect?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I am going to change this ;)

test/cli/cli_test.cpp Outdated Show resolved Hide resolved
@@ -96,8 +121,8 @@ TEST_F(cli_test, with_argument_verbose)

TEST_F(cli_test, with_out_file)
{
cli_test_result result = execute_app("fastq_to_fasta", data("in.fastq"), "-o", data("out.fa"));
seqan3::sequence_file_input fin{data("out.fa"), seqan3::fields<seqan3::field::seq, seqan3::field::id>{}};
cli_test_result result = execute_app("fastq_to_fasta", data("in.fastq"), "-o", workdir("out.fasta"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cli_test_result result = execute_app("fastq_to_fasta", data("in.fastq"), "-o", workdir("out.fasta"));
cli_test_result result = execute_app("fastq_to_fasta", data("in.fastq"), "-o", "out.fasta");

The app should be executed with the cwd = workdir.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! So I will use std::filesystem::current_path to set the work directory... But this would result in every test creating a (possibly empty) output directory, even if no output files are produced. Is that fine?

Copy link
Member

@marehr marehr Apr 7, 2020

Choose a reason for hiding this comment

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

I think rene and I thought that a cwd will be created only temporary for the current executed test case and that it will be deleted afterwards. If someone wants to keep the files he should use a debugger with break points and copy out the files manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest we offer a target make test_clean that deletes the output directory. We want to motivate test-driven development, so the output must be easily available. Users who do not need the files can delete them with this simple make command. Is it a solution you can agree with?

Copy link
Member

@marehr marehr Apr 7, 2020

Choose a reason for hiding this comment

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

Not sure how cmake will know what it needs to delete, but you are right. We have two different scenarios: someone who executes the test (does not want thousands of generated files that might be quite big) and the app-developer who wants to see the results when writing the test cases.

My gut feeling is that the tests will be executed way more (time-wise) in an automatic test (e.g. CI, does the app still build and produces my gold-standard results) setting than in a manual test where I care about the actual results. So I think that the default should be delete everything.

For example, cmake has an option

  --debug-trycompile           = Do not delete the try_compile build tree.
                                 Only useful on one try_compile at a time.

that keeps the intermediate files. Maybe we want something like this when executing the test.

Something like

> ./fastq2fasta_cli_test --keep-working-directories

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, that sounds like a good solution! I try implement this :)

Copy link
Member

Choose a reason for hiding this comment

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

We should do the --keep-working-directories on a later PR, because we need to change the infrastructure to allow parsing new arguments.

@joergi-w
Copy link
Member Author

I applied all suggestions in the third commit and then rebased the branch on master because of a merge conflict.

@joergi-w joergi-w requested a review from marehr April 14, 2020 07:12
Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

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

Sorry I forgot to submit the review 🙈

test/cli/cli_test.cpp Outdated Show resolved Hide resolved
ASSERT_NO_THROW(std::filesystem::remove_all(test_dir)); // delete the directory if it exists
ASSERT_NO_THROW(std::filesystem::create_directories(test_dir)); // create new empty directory
ASSERT_NO_THROW(original_wd = std::filesystem::current_path()); // store original wd
ASSERT_NO_THROW(std::filesystem::current_path(test_dir)); // change wd
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this works as intended, because setup is before the actual execution of the test.

Okay https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#assertion-placement says it would be okay: "put the fatal assertion in a SetUp/TearDown function".

See also https://github.com/google/googletest/blob/master/googletest/docs/faq.md#should-i-use-the-constructordestructor-of-the-test-fixture-or-setupteardown-ctorvssetup

I would just omit the ASSERT_NO_THROW. If it throws it throws. It generally gives better error messages if you let the exception throw.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's compare:

With ASSERT_NO_THROW it looks like this:

app-template/test/cli/cli_test.cpp:70: Failure
Expected: std::filesystem::current_path(test_dir) doesn't throw an exception.
  Actual: it throws std::exception-derived exception with description: "filesystem error: cannot set current path: No such file or directory".

without:

unknown file: Failure
C++ exception with description "filesystem error: cannot set current path: No such file or directory" thrown in SetUp().

I like in the second version that it reports the SetUp() function, but we do not see the file and line where the failure occurs. The ASSERT_NO_THROW command can be extended with further information like

ASSERT_NO_THROW() << "Failed to set " << test_dir << " as work directory."

which prints an extra line so the user can see which directory causes the problem. Therefore, I suggest to use the macro with additional information.

Then we see:

app-template/test/cli/cli_test.cpp:70: Failure
Expected: std::filesystem::current_path(test_dir) doesn't throw an exception.
  Actual: it throws std::exception-derived exception with description: "filesystem error: cannot set current path: No such file or directory".
Failed to set "build/app-template/test/output/cli_test.no_options" as work directory.

Copy link
Member

@marehr marehr Apr 16, 2020

Choose a reason for hiding this comment

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

I have a general problem with ASSERT_NO_THROW.

  • It adds visual noise to an expression and I find it harder to read code if every(!) single line is using it.
  • It is weird from a usage perspective. EXPECT_THROW makes sense, because I expect a throw. This is intended behaviour. A statement where I write explicitly EXPECT_NO_THROW is weird, because I already assume that every statement that is not EXPECT_THROW will not throw. So in tests I would never use that, because it does not add value.
  • ASSERT means I abort the execution, the exact same thing as if I would have thrown the exception. Generally try to avoid ASSERT and use EXPECT instead, because you want to know if every test case has the same setup problem or if only one test case had that problem.
  • It gives me the same exception message filesystem error: cannot set current path: No such file or directory

I understand that in this case the statements are like MAYBE_THROWS, but in reality it is more like SHOULD_NEVER_THROW (or is your routine wrong?).

I have to admit that it gives you the benefit to add additional information, like your "Failed to set " << test_dir << " as work directory.".

I can compromise here, if you put all the calls in a single function and use only one EXPECT_NO_THROW.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I get your point. I have chosen assert over expect, because the test should not execute further if there is no work directory (that's the difference according to the gtest doc).

I do not want to overcomplicate things, so I will just use the commands without any *_NO_THROW as you initially suggested.

Last question @marehr: Can I put the commands in a try-catch block like the following or better not?

try 
{
    4x std::filesystem commands without assert...
} 
catch (std::exception e)
{
    FAIL() << "Creating test directory " << test_dir << " failed: " + e.message();
}

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late reply. You can do that :)

@joergi-w joergi-w requested a review from marehr April 16, 2020 13:01
Comment on lines 59 to 77
void SetUp() override
{
::testing::TestInfo const * const info = ::testing::UnitTest::GetInstance()->current_test_info();
std::filesystem::path const test_dir{std::string{OUTPUTDIR} +
std::string{info->test_case_name()} +
std::string{"."} +
std::string{info->name()}};

ASSERT_NO_THROW(std::filesystem::remove_all(test_dir)) // delete the directory if it exists
<< "Failed to delete directory " << test_dir << ".";
ASSERT_NO_THROW(std::filesystem::create_directories(test_dir)) // create new empty directory
<< "Failed to create directory " << test_dir << ".";
ASSERT_NO_THROW(original_workdir = std::filesystem::current_path()) // store original wd
<< "Failed to query the current work directory.";
ASSERT_NO_THROW(std::filesystem::current_path(test_dir)) // change wd
<< "Failed to set " << test_dir << " as work directory.";
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void SetUp() override
{
::testing::TestInfo const * const info = ::testing::UnitTest::GetInstance()->current_test_info();
std::filesystem::path const test_dir{std::string{OUTPUTDIR} +
std::string{info->test_case_name()} +
std::string{"."} +
std::string{info->name()}};
ASSERT_NO_THROW(std::filesystem::remove_all(test_dir)) // delete the directory if it exists
<< "Failed to delete directory " << test_dir << ".";
ASSERT_NO_THROW(std::filesystem::create_directories(test_dir)) // create new empty directory
<< "Failed to create directory " << test_dir << ".";
ASSERT_NO_THROW(original_workdir = std::filesystem::current_path()) // store original wd
<< "Failed to query the current work directory.";
ASSERT_NO_THROW(std::filesystem::current_path(test_dir)) // change wd
<< "Failed to set " << test_dir << " as work directory.";
}
std::filesystem::path unique_test_workspace()
{
::testing::TestInfo const * const info = ::testing::UnitTest::GetInstance()->current_test_info();
std::filesystem::path const test_dir{std::string{OUTPUTDIR} +
std::string{info->test_case_name()} +
std::string{"."} +
std::string{info->name()}};
}
void create_test_workspace(std::filesystem::path const test_dir)
{
std::filesystem::remove_all(test_dir); // delete the directory if it exists
std::filesystem::create_directories(test_dir); // create new empty directory
original_workdir = std::filesystem::current_path(); // store original wd
std::filesystem::current_path(test_dir); // change wd
}
void restore_test_workspace()
{
std::filesystem::current_path(original_workdir);
}
void SetUp() override
{
std::filesystem::path const test_dir = unique_test_workspace();
EXPECT_NO_THROW(create_test_workspace(test_dir)) << "Failed to set-up new workspace [" << test_dir << "].";
}

Comment on lines 79 to 80
ASSERT_NO_THROW(std::filesystem::current_path(original_workdir)) // restore original wd
<< "Failed to set " << original_workdir << " as work directory.";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ASSERT_NO_THROW(std::filesystem::current_path(original_workdir)) // restore original wd
<< "Failed to set " << original_workdir << " as work directory.";
EXPECT_NO_THROW(restore_test_workspace()); // restore original wd

ASSERT_NO_THROW(std::filesystem::remove_all(test_dir)); // delete the directory if it exists
ASSERT_NO_THROW(std::filesystem::create_directories(test_dir)); // create new empty directory
ASSERT_NO_THROW(original_wd = std::filesystem::current_path()); // store original wd
ASSERT_NO_THROW(std::filesystem::current_path(test_dir)); // change wd
Copy link
Member

@marehr marehr Apr 16, 2020

Choose a reason for hiding this comment

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

I have a general problem with ASSERT_NO_THROW.

  • It adds visual noise to an expression and I find it harder to read code if every(!) single line is using it.
  • It is weird from a usage perspective. EXPECT_THROW makes sense, because I expect a throw. This is intended behaviour. A statement where I write explicitly EXPECT_NO_THROW is weird, because I already assume that every statement that is not EXPECT_THROW will not throw. So in tests I would never use that, because it does not add value.
  • ASSERT means I abort the execution, the exact same thing as if I would have thrown the exception. Generally try to avoid ASSERT and use EXPECT instead, because you want to know if every test case has the same setup problem or if only one test case had that problem.
  • It gives me the same exception message filesystem error: cannot set current path: No such file or directory

I understand that in this case the statements are like MAYBE_THROWS, but in reality it is more like SHOULD_NEVER_THROW (or is your routine wrong?).

I have to admit that it gives you the benefit to add additional information, like your "Failed to set " << test_dir << " as work directory.".

I can compromise here, if you put all the calls in a single function and use only one EXPECT_NO_THROW.

Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

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

I could compromise on this solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants