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.

Sign up for GitHub

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

Jump to bottom

Use pFUnit 4 for testing #70

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use pFUnit 4 for testing #70

wants to merge 4 commits into from

Conversation

Mikolaj-A-Kowalski
Copy link
Collaborator

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski commented Aug 21, 2023 •
edited
Loading

This aims to address #10 and switch the pFUnit to the version 4. Furthermore, it turns out that warnings were not enable when building tests. Consequently the warnings that are now raised are addressed.

Note:

  • Since we remove the custom FindPFUnit.cmake the hint enviromental variable has changed from PFUNIT_INSTALL to PFUNIT_DIR.
  • The GitHub Actions will fail due to old pFUnit in the containers (they will need to be updated, I am not sure how to make it in a clean way)
  • pfUnit seems to have a bug that creates an unused variable when processing a Test Fixture. Sadly this means that the warning needs to be suppressed.
  • Tests use the same flags as SCONE. Sadly it means that the optimisation is turned on which makes the tests compile much slower.

In short term we should also aim to update the CMake configuration and use FetchContent to download the pFUnit automatically. I did not do this in this PR to keep it smaller. Not sure if this is a good idea or bad.

P.S. Should definitely NOT be merged before the workshop since it will cause havoc...

@Mikolaj-A-Kowalski
Switch to use pFUnit 4.7
f4e5abd 
Includes necessary modifications to CMakeLists.txt and the tests
themselves.

At the moment produces large number of warnings from the tests and
CMake.
@Mikolaj-A-Kowalski
Remove easy to fix warnings.
8612baa 
Mostly unused variables and unintended conversions.
@Mikolaj-A-Kowalski
Supress unused variable warning due to pFUnit bug
3170b67
@Mikolaj-A-Kowalski Mikolaj-A-Kowalski self-assigned this Aug 21, 2023
@Mikolaj-A-Kowalski Mikolaj-A-Kowalski changed the title Draft: Use pFUnit 4 for testing Use pFUnit 4 for testing Aug 21, 2023
@Mikolaj-A-Kowalski
Change the Docker image used for tests
3cf6571 
Now uses a version with PFUnit 4.7.3
@Mikolaj-A-Kowalski
Copy link
Collaborator Author

@valeriaRaffuzzi We probably need to start to move this PR forward.
I have updated the Github actions settings so the tests are run with images that come with pre-installed pFUnit 4.7.3, hence the automated tests pass now without any problem.

The main issues to figure out at the moment is:

  1. Should we go over and do a review now or should I rebase on the current main before we do
  2. Do we accept the price of longer compilation time for the tests (now they run with optimisations enabled) or should I play with CMake settings a bit more to go back to compiling them with -O0
  3. Of course integrating this PR will cause havoc, but there is no way around it. What is the best way to proceed to minimise disruption to everybody.

@valeriaRaffuzzi
Copy link
Member

  1. I don't have a strong preference, but I don't think it's necessary to rebase.
  2. How long are we talking about? The compilation is already quite slow, so it mainly depends on how tricky it is to change those flags.
  3. I don't think it will be a big deal. It's just about downloading the latest pFUnit, right? Maybe the most painless option would be to download automatically the right version of pFUnit as you were mentioning above, but I don't think it will be as bad as you say if we don't do that.

Otherwise: If the name of the PFUNIT environmental variable has changed we should update the Installation doc as well.

@Mikolaj-A-Kowalski
Copy link
Collaborator Author

Mikolaj-A-Kowalski commented Nov 24, 2023 •
edited
Loading

  1. Ok. We will just need to rebase before merge to make sure that all tests which were added since this branch diverged from main were updated to new syntax.
  2. Ouch 🙃 ! And here I was thinking that SCONE compiles fast! 🥲 New: 4m24s; Old 2m40s on my laptop with single core.
  3. It is mostly that all developments done before the this branch is merged will need to update the syntax of the test files and there is a good chance for many marge conflicts in tests!

P.S.: You are absolutely right for the Installation! Sorry, I completely forgot about it.

ChasingNeutrons
ChasingNeutrons reviewed Apr 18, 2024
View reviewed changes
DataStructures/Tests/charMap_test.f90
@@ -198,7 +196,7 @@ subroutine testLooping(this)
character(nameLen) :: tKey

! Initialise parameters
KEYS_PAST = "This is not a Key. It's picture of a key!"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?!

ChasingNeutrons
ChasingNeutrons approved these changes Apr 18, 2024
View reviewed changes
Copy link
Collaborator

@ChasingNeutrons ChasingNeutrons left a comment

Choose a reason for hiding this comment

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

Very very belatedly... Thanks for doing this! It looks like it was surprisingly straightforward, other than a lot of name changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChasingNeutrons ChasingNeutrons ChasingNeutrons approved these changes

Assignees

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski

Labels
testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mikolaj-A-Kowalski @valeriaRaffuzzi @ChasingNeutrons