Test fwd_step_log_open

[dafeda]

Issue
Resolves #2387

Will all code under WHEN always run even if something crashes?
I would like to always run the following,

            fwd_step_log_free(fwd_step_log);
            std::filesystem::remove_all(log_file_path);

but am not sure if this is the right way to go about it.

[eivindjahren]

Issue Resolves #2387

Will all code under WHEN always run even if something crashes? I would like to always run the following,

            fwd_step_log_free(fwd_step_log);
            std::filesystem::remove_all(log_file_path);

but am not sure if this is the right way to go about it.

Unfortunately no, the way to ensure that remove_all is called is to do an RAII object for the log which you then use as a fixture:

class TmpLog
{
   public:
   string log_file_path;
   fwd_step_log_type *fwd_step_log

   TmpLog()
     : log_file_path( "tmp_fwd_step_log_open/test.txt" )
   {
       fwd_step_log = fwd_step_log_alloc();
       fwd_step_log_set_log_file(fwd_step_log, log_file_path);
       fwd_step_log_open(fwd_step_log);
   }
   ~TmpLog()
   {
       fwd_step_log_free(fwd_step_log);
       std::filesystem::remove_all(log_file_path);
   }
};

I believe that is the correct solution, but perhaps @dotfloat has some interesting more modern c++ thoughts on this.

[dafeda]

I believe that is the correct solution, but perhaps @dotfloat has some interesting more modern c++ thoughts on this.

Thanks!
I've pushed a version using this, but am not sure if this is following best practice.

[eivindjahren]

I certainly like it, but perhaps it would be best to ask @dotfloat for a review aswell.

[eivindjahren]

Ah, I did manage to remember something. This is still not 100% foolproof. Things can go south and the object destructor never called so the folder never removed but it would have to be really catastrophic (like pulling the cable). So I suggest you use tmpfile or tmpnam so that a new temporary file (or directory) is always created.

[pinkwah]

So, technically, you're not using RAII (at least not directly). You're using catch2's test fixture functionality. If you were to RAII, you'd write:

TEST_CASE("..", "[foo]") {
    TmpLog tmp_log;

    // ...
}

This is probably okay. But to answer @eivindjahren , modern C++ goes full speed ahead wrt. RAII, so they are definitely the way to go.

[eivindjahren]

So, technically, you're not using RAII (at least not directly). You're using catch2's test fixture functionality. If you were to RAII, you'd write:

TEST_CASE("..", "[foo]") {
    TmpLog tmp_log;

    // ...
}

This is probably okay. But to answer @eivindjahren , modern C++ goes full speed ahead wrt. RAII, so they are definitely the way to go.

Good to know that I am not completely out of the loop to suggest RAII here. I think we can call this RAII pattern even though we use it as a fixture.

As you noted when we talked about this. Even calling util (which this code is fond of) will break all our efforts of trying to clean up unless we put it into a abort signal handler (lets not do that). So I suggest adding that tmpnam is used to generate a unique name on /tmp everytime so that the file is never reused.

[pinkwah]

I agree wrt. creating a new directory. I would suggest stealing the tmp_path_factory code from pytest and converting it to C++.

[eivindjahren]

I agree wrt. creating a new directory. I would suggest stealing the tmp_path_factory code from pytest and converting it to C++.

Could be, as the original author of that I hesitated to suggest it since it might be overkill and it has a few weaknesses. Since it changes the entry point to the tests the compile time goes up a bit because catch2 is not optimized for that usage. Also we might want to resolve equinor/resdata#821 first.

[dafeda]

Did not think of RAII as it's new to me.

I read up on it a bit but can't see what kind of problems we might end up having by using what we have now.
From my understanding the destructor of TmpLog will get called if something fails inside TEST_CASE_METHOD, which is what we want.

What can go wrong?

[eivindjahren]

Did not think of RAII as it's new to me.

I read up on it a bit but can't see what kind of problems we might end up having by using what we have now. From my understanding the destructor of ´TmpLogwill get called if something fails insideTEST_CASE_METHOD`, which is what we want.

What can go wrong?

RAII will handle things correctly for the normal error handling stuff, however that still leaves signals and catastrophic failures. If the program is given SIGTERM, SIGABORT, SIGSEGV or any of the other signals, the destuctor will not be called. If you pull out the power cord before you have a chance to perform the destructor, it will not magically happen.

So the cases we are most worried about is SIGABORT which our good friend util_abort will do for us, SIGSEGV because memory violations happen all the time, and SIGTERM because we quite often do that via Jenkins. It is possible to install signal handlers, but that would be an incredibly flaky and complicated solution.

So the solution proposed is to instead be robust to what happens when we are unable to clean up. That involves (1) writing our temporary file to /tmp (or std::filesystem::temp_directory_path if you want to be portable, the OS manages the clean up of that) and (2) creating a unique filename every time (std::tempnam handles that).

pytest has a pretty cool fixture for doing this which creates a directory /tmp/pytest-of-{$USER}/pytest-{$RUN_NUMBER}/{$TESTNAME}/ for every test run. It keeps the 5 most recent runs before it starts cleaning up. We sort-of recreated that for ecl https://github.com/equinor/ecl/blob/master/lib/tests/tmpdir.hpp. You can see it used here: https://github.com/equinor/ecl/blob/master/lib/tests/test_util.cpp. It hooks into the runner to make it possible to remove on request, rather than pytests behavior of keeping the 5 most recent runs. Probably we want to change that behavior.

[dafeda]

Ah, I did manage to remember something. This is still not 100% foolproof. Things can go south and the object destructor never called so the folder never removed but it would have to be really catastrophic (like pulling the cable). So I suggest you use tmpfile or tmpnam so that a new temporary file (or directory) is always created.

Generating random names is a good idea, thanks.

clang complains about tmpnam though:

...warning: 'tmpnam' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of tmpnam(3), it is highly recommended that you use mkstemp(3) instead. [-Wdeprecated-declarations]

mkstemp and tmpfile return handlers and what I want is a random filename.

Could perhaps create a "unique" filename by appending a timestamp or something similar, but I don't like that.

Ideas?

[eivindjahren]

Ah, I did manage to remember something. This is still not 100% foolproof. Things can go south and the object destructor never called so the folder never removed but it would have to be really catastrophic (like pulling the cable). So I suggest you use tmpfile or tmpnam so that a new temporary file (or directory) is always created.

Generating random names is a good idea, thanks.

clang complains about tmpnam though:

...warning: 'tmpnam' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of tmpnam(3), it is highly recommended that you use mkstemp(3) instead. [-Wdeprecated-declarations]

mkstemp and tmpfile return handlers and what I want is a random filename.

Could perhaps create a "unique" filename by appending a timestamp or something similar, but I don't like that.

Ideas?

mkstemp has its own problem that it is posix standard only. You can get the name of the file by using std::fileno and fctl, but I am not sure whether mkstemp guarantees that the file remains open after you close it.

If we include boost we could do

using namespace boost::filesystem;

path ph = temp_directory_path() / unique_path();
create_directories(ph);

[dafeda]

If we include boost we could do

using namespace boost::filesystem;

path ph = temp_directory_path() / unique_path();
create_directories(ph);

Seems a bit overkill to include boost, but maybe..
What if we create a filename using a uuid using for example this:

https://conan.io/center/stduuid

[eivindjahren]

If we include boost we could do

using namespace boost::filesystem;

path ph = temp_directory_path() / unique_path();
create_directories(ph);

Seems a bit overkill to include boost, but maybe.. What if we create a filename using a uuid using for example this:

https://conan.io/center/stduuid

I don't know, we might want to use boost more in any case, and if we use stduuid then we have to be responsible for platform specifics of whether the filename created is valid. I think that is not going to be a problem though, so I have nothing against the uuid solution.

[eivindjahren]

Should be noted that the reason not to use tmpnam is because of a race condition (ie. two processes creating the same tempfile name because they notice it is available before either has had a chance to claim it), so we are recreating that same scenario now by using stduuid.

That is one reason why pytest uses the folder structure it does for its tmpdir fixture.

[dafeda]

This function has been deleted, but the work can be used in testing other functions that write to disk.