Test Modules for Reading and Writing

[kursatyurt]

This PR adds some basic Ctest modules to check VTK outputs.

There are several tests included

1 - Check the capability of writing scalar data VTK files.
2 - Check the output of scalar data VTK file.
3-4-5-6 are variants of tests 1-2 for 2D vector and 3D vector data.
7 - 8 - 9 are reading tests they read some basic reference files and check reader values.

It also contains some bug fixes in mesh.cpp related to VTK library.

[davidscn]

Nice, thank you the tests work on my machine.
However, I get plenty of these warnings when building the prgram

mesh.cpp:115:50: warning: narrowing conversion of ‘cell->vtkCell::GetPointId(0)’ from ‘vtkIdType’ {aka ‘long long int’} to ‘long unsigned int’ [-Wnarrowing]
  115 |       std::array<size_t, 3> elem{cell->GetPointId(0), cell->GetPointId(1), cell->GetPointId(2)};

I guess the arrays you use in several places with size_t need to be vtkIdType

[fsimonis]

Writing your own test executables doesn't scale, so let's use a testing framework instead.
As ASTE already relies on Boost.Container, we could use Boost.Test for the testing.

Our baseline is boost 1.65.1 (Ubuntu 18.04 LTS). Here are some resources:

• Documentation Overview
• Let's use the shared library version. You can #define BOOST_TEST_DYN_LINK using CMake for the entire executable. The testing main should be in a stand-alone file:

  #define BOOST_TEST_MODULE ASTE
  #define BOOST_TEST_DYN_LINK
  #include <boost/test/unit_test.hpp>

• You should only have to link to Boost::unit_test_framework and Boost::boost (boost container) using CMake.

[davidscn]

Writing your own test executables doesn't scale, so let's use a testing framework instead.
As ASTE already relies on Boost.Container, we could use Boost.Test for the testing

Scale to what? I don't expect a massive test framework within this small project. We should keep the effort for a port to boost tests in mind. Do you really think it is worth the effort? IMHO, some basic tests as added here should be perfectly fine. Also, upcoming ASTE features like the replay mode and an ASTE tutorial are in my opinion more important then a boost test framework.

[fsimonis]

Scale to what?

Tests need to be maintained and it is easier to maintain tests written using a well known framework than having to understand a custom framework first.
We also use boost.test in preCICE, so core developers are already familiar with the basics.
Instead of reinventing the wheel, let's reuse the one we already know.

I don't expect a massive test framework within this small project.

Neither do I and relying on an existing framework keeps the tests short, readable and maintainable.

Also, upcoming ASTE features like the replay mode and an ASTE tutorial are in my opinion more important then a boost test framework.

I agree. Migrating to Boost.test is not a lot of work, so let's just do it.

All of this really applies to the logging framework too, but this migration will be more work as we already use logging in many places..

[kursatyurt]

mesh.cpp:115:50: warning: narrowing conversion of ‘cell->vtkCell::GetPointId(0)’ from ‘vtkIdType’ {aka ‘long long int’} to ‘long unsigned int’ [-Wnarrowing]
  115 |       std::array<size_t, 3> elem{cell->GetPointId(0), cell->GetPointId(1), cell->GetPointId(2)};

I guess the arrays you use in several places with size_t need to be vtkIdType

It seems at some point we need a narrowing process. Since ASTE stores them as size_t I kept this convention. preCICE interface accepts int type whether they converted from long long int to unsigned int does not make any sense in the end right? Should I directly apply a static_cast here to convert int?

[davidscn]

mesh.cpp:115:50: warning: narrowing conversion of ‘cell->vtkCell::GetPointId(0)’ from ‘vtkIdType’ {aka ‘long long int’} to ‘long unsigned int’ [-Wnarrowing]
  115 |       std::array<size_t, 3> elem{cell->GetPointId(0), cell->GetPointId(1), cell->GetPointId(2)};

I guess the arrays you use in several places with size_t need to be vtkIdType

It seems at some point we need a narrowing process. Since ASTE stores them as size_t I kept this convention. preCICE interface accepts int type whether they converted from long long int to unsigned int does not make any sense in the end right? Should I directly apply a static_cast here to convert int?

I see, this is actually a deficiency of preCICE. The problem is that the current state 'looks' a bit like an accident when the compiler warnings pop up. Please use static casts and add a comment in the code why you statically convert the type here.

[fsimonis]

In order to test the mesh_writing, I would propose to write the test in a way that it first writes a mesh, then reads the vtk and run checks on the points and data.
This also makes it work with both ASCII and Binary vtk files.

[fsimonis]

I see, this is actually a deficiency of preCICE.

@davidscn There is actually an open issue regarding this precice/precice#887.

[davidscn]

@kursatyurt ynce you decided how to proceed in the discussion, we can update the main branch here and move on to the tutorial. If you do not add a GitHub action for this test, I will do later on.

[kursatyurt]

@kursatyurt once you decided how to proceed in the discussion, we can update the main branch here and move on to the tutorial. If you do not add a GitHub action for this test, I will do later on.

The clearest one is using int arrays instead of size_t arrays to keep it consistent with the preCICE interface. I pushed the changes.

I tried to add one action but it is not working. If you can add an action and close the request it would be nice.

[davidscn]

I will add one in a later PR. Could you then as a last step add an assertion for the assumption of continuous indices as mentioned above?

[fsimonis]

I propose a solution to this type juggling in #29 (comment)

[davidscn]

I don't have enough to argue here, so I will let the discussion up to you.