Skip to content

Test fixtures 413 #421

New issue

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

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

Already on GitHub? # to your account

Merged
merged 5 commits into from
Jul 24, 2019
Merged

Conversation

zbeekman
Copy link
Contributor

Fixes #413

Also, re-enables & fixes some validation of outputs created by unit tests.

@zbeekman
Copy link
Contributor Author

Also, removes reliance on diff to check output for regressions.

@zbeekman
Copy link
Contributor Author

Assuming no test failures (or only due to #422) this PR should be ready to merge. If you have any questions, or need more info, LMK.

@codecov-io
Copy link

codecov-io commented Jul 22, 2019

Codecov Report

Merging #421 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #421   +/-   ##
=======================================
  Coverage   89.51%   89.51%           
=======================================
  Files           3        3           
  Lines        5305     5305           
=======================================
  Hits         4749     4749           
  Misses        556      556

@zbeekman
Copy link
Contributor Author

@jacobwilliams let me know if there's anything you want me to explain or cleanup. Otherwise, I believe that this is ready to merge.

CMakeLists.txt Outdated
@@ -14,6 +14,9 @@ cmake_minimum_required ( VERSION 2.8.8 FATAL_ERROR )
# Use MSVS folders to organize projects on windows
set_property(GLOBAL PROPERTY USE_FOLDERS ON)

option(JSON_FORTRAN_USE_OpenCoarrays
"Build VTKmofo with support for linking against OpenCoarray programs" OFF)
Copy link
Owner

Choose a reason for hiding this comment

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

VTKmofo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Derp.... I saw that and meant to fix it but then lost track of it. (https://github.com/porteri/vtkmofo)

@jacobwilliams
Copy link
Owner

Do we need the OpenCoarrays stuff in the Cmake file? If it's useful we can keep it, can you provide me with some text I can put in the release notes explaining what that is for?

@jacobwilliams
Copy link
Owner

Can you pull latest master to get the change from #422?

@zbeekman
Copy link
Contributor Author

Do we need the OpenCoarrays stuff in the Cmake file? If it's useful we can keep it, can you provide me with some text I can put in the release notes explaining what that is for?

The problem arises when you have a coarray code and you want to link JSON-Fortran. If JSON-Fortran is compiled without -fcoarray=caf the machine code and module files generated by GFortran is different and you get build time errors (differences in the module files) or link time errors. It's off by default, and it just provides an option to turn it on. This is really a bug in GFortran, AFAICT.

@zbeekman zbeekman force-pushed the test-fixtures-413 branch from 11c1e8d to 2ddc226 Compare July 23, 2019 16:43
zbeekman added 2 commits July 23, 2019 12:56
 - Passing `-fcoarray=lib` to gfortran causes the compiler to generate
   different modules & symbols. This is likely a bug in gfortran, but,
   a consequence is that you need to build libraries you link to with the
   same `-fcoarray=...` flag.
@zbeekman zbeekman force-pushed the test-fixtures-413 branch from 2ddc226 to a7e7978 Compare July 23, 2019 16:57
as the coarray runtime implementation. Use the
`-DJSON_FORTRAN_USE_OpenCoarrays:BOOL=ON` option to cmake to enable
this. (NOTE: The fact that this is required may be a bug in
GFortran.)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacobwilliams how's this for the explanation? If you prefer, I can remove this option and users can manually tweak CMake flags if they need to.

Copy link
Owner

Choose a reason for hiding this comment

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

That sounds fine. I don't mind keeping it if it's useful.

@jacobwilliams jacobwilliams merged commit ffca283 into jacobwilliams:master Jul 24, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate test setup/teardown to fixtures
3 participants