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

#32: vt-tv: Improve CI #85

Merged
merged 477 commits into from
Aug 26, 2024
Merged

#32: vt-tv: Improve CI #85

merged 477 commits into from
Aug 26, 2024

Conversation

tlamonthezie
Copy link
Contributor

@tlamonthezie tlamonthezie commented Jul 8, 2024

Fixes #32
Fixes #92
Fixes #103
Might fix #69

Important! Dockerhub repository is modified from that PR (now same that vt one). Base docker images are used for unit tests and are not already pushed to the new repository which will generate failures on CI until merge is done.

One of the latest run (using old repository) can be found at https://github.com/DARMA-tasking/vt-tv/actions/runs/10177046856

After merging please call the pushdockerimage action to push base images (with VTK) to the new Dockerhub repository. Until that CI will fail

This PR includes:

Unit tests refactoring and writing:

  1. Remove obsolete code in the tests & example directories
  2. Move GoogleTest and its & fetch script to lib directory + update the fetch script.
  3. Re-organize the tests directory and update CMakeLists under tests/unit and build tests in a single executable for Google test optimal use.
  4. Add unit tests mainly for the api classes
  5. Add TESTING.md documentation to get started with testing.
  6. Add root build.sh script capable for local use as well as for ci use with some options for build, coverage and test run. get more info with build.sh --help. This provides for example a way to generate coverage html report, JUnit report etc.
  7. Add simple diff test for CCM example PNG output with some tolerance. See tests/test_image.sh (Call internally a python module for now to diff between actual and expected png image). (This is called by the ParseRenderTest class after rendering with the ccm_example0.png generated file).
  8. Remove ctest calls to use only GTest directly (Test compiled in a file)
  9. Reorganize tests data files and config files and re-generate lb_synthetic_data using the create_synthetic_attributes_data.py script (and reorganize generated attributes alphabetically in the python script)

CI:

  1. Add xvfb as graphics display in CI.
  2. Make python bindings test working in CI and include in the Build & Test pipeline workflow (remove test bindings workflow). See ci/docker/build-and-test.dockerfile and called shell script for details.
  3. Include Test results & Coverage in Github step summary of job run.
  4. Upgrade external used github actions (some versions were deprecated)
  5. Make docker image configurable (base image, compiler, VTK version, Python version).
    5.1 Configurable base image: ci/docker/make-base.dockerfile used by the .github/workflows/pushbasedockerimage.yml . The pushbasedockerimage contains now an input to select the image to build and push to DockerHub.
    5.2 Configurable vt-tv build & test image ci/docker/build-and-test.dockerfile used by the .github/workflows/build-and-test.yml workflow
    5.3 Provide test matrix using the 3 images in the build & test workflow:
    • ubuntu_22.04-gcc_11-vtk_9.2.2-py_3.8 (Build, Test)
    • ubuntu_22.04-clang_14-vtk_9.2.2-py_3.8 (Build, Test)
    • ubuntu_22.04-gcc_12-vtk_9.3.0-py_3.8 (Build, Test, Coverage)
  6. Add CMake options (enable tests, enable coverage)

Because docker images are more scalable it will be easier to add later additional configurations to the pushbasedockerimage.yml workflow and to add it to tests in the build-and-test.yml images to build vt-tv on and to run tests on.
Additionnaly the ci folderhas been re-organized and docker images are now in the newci/docker` directory

Additionally added fix from #92 to make checks successful for clang.

@tlamonthezie tlamonthezie self-assigned this Jul 8, 2024
@tlamonthezie tlamonthezie force-pushed the 32-vt-tv-improve-ci branch from d3649ce to 7d4bb07 Compare July 9, 2024 09:55
@tlamonthezie tlamonthezie force-pushed the 32-vt-tv-improve-ci branch 3 times, most recently from e00116d to d30e481 Compare July 23, 2024 18:32
@tlamonthezie tlamonthezie marked this pull request as ready for review July 25, 2024 16:57
Copy link
Contributor

@cwschilly cwschilly left a comment

Choose a reason for hiding this comment

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

Mostly minor changes--everything looks great on the whole. I do think we should test the content of the mesh files though.

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/find-trailing-whitespace.yml Outdated Show resolved Hide resolved
.vscode/c_cpp_properties.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
tests/unit/render/test_render.cc Outdated Show resolved Hide resolved
tests/unit/render/test_render.cc Outdated Show resolved Hide resolved
tests/unit/render/test_render.cc Outdated Show resolved Hide resolved
tests/unit/render/test_render.cc Outdated Show resolved Hide resolved
tests/unit/util.h Outdated Show resolved Hide resolved
Copy link
Contributor

@ppebay ppebay left a comment

Choose a reason for hiding this comment

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

Please create follow-on issues @tlamonthezie for aspects that are not covered yet by CI but that we discussed at last meeting.

@tlamonthezie tlamonthezie mentioned this pull request Jul 29, 2024
2 tasks
@tlamonthezie
Copy link
Contributor Author

tlamonthezie commented Jul 29, 2024

Please create follow-on issues @tlamonthezie for aspects that are not covered yet by CI but that we discussed at last meeting.

Hi @ppebay I created an issue to continue to work on the CI/Tests see #95 . Some additional things than those discussed have also been added.

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/pushbasedockerimage.yml Outdated Show resolved Hide resolved
.github/workflows/pushbasedockerimage.yml Outdated Show resolved Hide resolved
.github/workflows/pushbasedockerimage.yml Outdated Show resolved Hide resolved
.vscode/c_cpp_properties.json Outdated Show resolved Hide resolved
src/vt-tv/render/render.cc Outdated Show resolved Hide resolved
tests/data/ccm_example/data.0.json Outdated Show resolved Hide resolved
tests/data/ccm_example/data.1.json Outdated Show resolved Hide resolved
@nlslatt
Copy link
Contributor

nlslatt commented Aug 5, 2024

This is a huge PR. You might consider breaking it up into multiple PRs if it gets difficult to get everything passing.

@cwschilly cwschilly force-pushed the 32-vt-tv-improve-ci branch from b79b3db to be002d7 Compare August 6, 2024 20:57
@cwschilly cwschilly requested review from ppebay and nlslatt August 7, 2024 15:41
@cwschilly cwschilly force-pushed the 32-vt-tv-improve-ci branch from 53df383 to 33ca721 Compare August 7, 2024 15:56
Copy link
Contributor

@nlslatt nlslatt left a comment

Choose a reason for hiding this comment

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

I noticed that style varies from file to file and I wonder if we should implement vt's style guidelines here as well, perhaps as a separate PR so we can get this merged sooner rather than later.

vt has a tool that will check/correct the header comments at the tops of files, including setting the correct filenames. There are more files that need to be corrected than I marked.

tests/unit/util.h Outdated Show resolved Hide resolved
tests/unit/generator.h Outdated Show resolved Hide resolved
tests/unit/api/test_object_info.cc Outdated Show resolved Hide resolved
tests/unit/api/test_object_work.cc Outdated Show resolved Hide resolved
@cwschilly cwschilly requested a review from nlslatt August 14, 2024 19:08
@cwschilly cwschilly force-pushed the 32-vt-tv-improve-ci branch from 58e03bc to 74e084b Compare August 14, 2024 20:38
@cwschilly cwschilly merged commit 9f8e5a0 into master Aug 26, 2024
8 checks passed
@cwschilly cwschilly deleted the 32-vt-tv-improve-ci branch August 26, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants