smash-transport / smash-hadron-sampler

Sample hadrons from vHLLE hydrodynamics output for SMASH afterburner.
Other
1 stars 7 forks source link

Sass/add tests #15

Closed nilssass closed 8 months ago

nilssass commented 9 months ago

This PR adds unit tests to the sampler including one first test for gen::index44(...)

yukarpenko commented 9 months ago

I don't quite get the point with the unit test for index44(). Do you want to test whether integer addition and multiplication works OK? If it doesn't I think it would signal a dead CPU ...

nilssass commented 9 months ago

Hi @yukarpenko, the philosophy of unit tests is to ensure that all parts of a code work as expected. This might sound trivial at some places but especially if information is passed between several stages, one would be surprised how many errors one does not anticipate from even simple functions. In this very case the unit test for index44() runs through all possible valid variables and ensures that it returns the expected values. This does not change the behavior of the code itself, it just builds test executables that can be ran after changes in the code to ensure that nothing broke.

However, this PR is mainly meant to start tests and start with the easiest example in gen(). This is because I am currently working on sampling spin from the hypersurface, meaning to add quite some functions that interact with the code at several places. For these functions I definitely need tests to make sure that every possible error is caught and does not brake the code.

I hope this clarified my need for this PR :)

yukarpenko commented 8 months ago

ok it is understandable if it is only the start with the unit tests. I would expect that there are much less trivial places in the code which would benefit from such.

yukarpenko commented 8 months ago

A git thing: from my side, I see the error "Merging is blocked The base branch does not allow updates." Were some branch setting changed? Or it was like that but noone was doing a merge into the main branch yet?

NGoetz commented 8 months ago

Yes merging is indeed blocked because a year ago we decided to have different main and develop branches. This is necessary because of the close interlocking of SMASH and the sampler, so if SMASH updates on develop, we might need changes in the sampler too, but we don't want the main branch to be broken wrt tagged SMASH. Therefore, features are to be merged into the develop branch.

nilssass commented 8 months ago

ok it is understandable if it is only the start with the unit tests. I would expect that there are much less trivial places in the code which would benefit from such.

Yes, this is definitely meant to have some starting point. I didn't want to add an empty file, so I took the easiest case, just to have it not empty and to set up the structure so that people know how to proceed. In the future there will be way more non trivial tests and also tests for the other files.

nilssass commented 8 months ago

Then I will change this PR and merge it into develop

nilssass commented 8 months ago

@yukarpenko I have changed the base branch, so that this PR will be merged into develop. Do you agree to merge it?

yukarpenko commented 8 months ago

Fine with me.

nilssass commented 8 months ago

Great, thank you!