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.

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

Adding regression tests #195

Closed
wants to merge 27 commits into from
Closed

Conversation

shimwell
Copy link
Collaborator

@shimwell shimwell commented May 23, 2024

Description

I've produced CSG files for all the step files in the repo and added these to a folder in the tests folder. Now whenever we make a PR a regression test will run that compares all the CSG files converted during the existing CI tests with those in the regression test folder. This takes less than a second so we won't notice any CI time increase.

One of the common pains of regression tests is that from time to time we will find a bug and fix it which changes the CSG files in a good way. However this will fail the regression tests as the test just checks if the newly produced CSG files match the older files.

To help smooth out this annoyance I've include a script tests/update_regression_test_files.py that will remake all the regression test files.

Most of the files added in this PR are the regression test CSG files (208 files) however to help with the review the important files edited in this PR are

  • tests/update_regression_test_files.py
  • tests/test_convert.py

Note that while I was making this I had to do some messy Path manipulation to ensure each stp file in the testing/inputSTEP folder has its own folder with no naming overlaps. One thing I would like to do in the future is to rename the files in the testing/inputSTEP folder in a better way so that we don't have files with the same name even when they are in different subfolders. Also if I had the ability to specify different file names stems for the CSG files then that would also make the Paths more elegant and concise (which makes me keen on sperate export_csg methods for each MC code). So please excuse the long Path operations as this is something I can clean up in a following PR and I want to keep this one on topic.

Fixes issue

helps with issue #46

Checklist

  • I'm making a PR from a feature branch on my fork into GEOUNED-org/GEOUNED/dev branch
  • I have followed PEP8 style guide for Python or run a formatter such as black or ruff format on my code
  • I have added tests that prove my fix is effective or that my feature works

@shimwell
Copy link
Collaborator Author

Looks like the files are not perfectly reproducible and I'm seeing lines in the regression test not matching the lines in the CI produced files

Errors like this are appearing, where the coefficients for the surfaces don't fully match but are very close as mentioned in this issue

  - surf 3 plane  6.4278761e-01 -7.6604444e-01  4.7675313e-14  9.4730000e+02
  ?                                                  -- ^
  + surf 3 plane  6.4278761e-01 -7.6604444e-01  4.7671611e-14  9.4730000e+02

this lack of reproducibility makes regression tests difficult so perhaps we should see if it is possible to make the files fully reproducible prior to the regression testing.

Or alternatively we can introduce some tolerance on numbers so they don't have to match perfectly

Any comments on the preferred direction would be appreciated

@shimwell shimwell marked this pull request as draft May 25, 2024 06:37
@shimwell
Copy link
Collaborator Author

I've added some extra code to the tests that will split each line up and check each token matches. if the token is a float then there is some tolerance on the checking (1e-6)

@shimwell shimwell closed this by deleting the head repository Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant