Skip to content
This repository has been archived by the owner on Dec 18, 2023. It is now read-only.

Move GTest: src/beanmachine/*/tests/ -> tests/*/ #1742

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

Conversation

ntfrgl
Copy link
Contributor

@ntfrgl ntfrgl commented Oct 10, 2022

Motivation

In preparation for a refactoring of the Bean Machine test suite, #1674 and #1711 moved the Pytest modules (BM/Python) into a dedicated tests/ directory. This is now followed by a corresponding move of the GTest suite (BMG/C++).

In addition, a new CMake build of BMG defines an entry point for the GTest suite to be executed from the GitHub Actions CI --- previously, this was only configured in the internal build system.

The choice of CMake as the testing build system is based on its official support both by GTest and by Vcpkg. Separate linking (BMG+Pybind11, BMG+BMG_test+GTest) is preferable over full linking (BMG+Pybind11+BMG_test+GTest), because it avoids adding new build requirements (i.e., CMake) to the Python package and allows for testing BMG/C++ independently of BM/Python.

Changes proposed

  • C++ tests are relocated from src/ to tests/. At present, this does not affect minibmg/tests/.
  • The Python package definition is updated, and compilation of the Pybind11 extension is parallelised [1].
  • A CMake build of BMG with GTest integration [2] is defined, independently of the Pybind11 integration.
  • The new GTest entry point is added to the GitHub Actions.

[1] https://pybind11.readthedocs.io/en/latest/compiling.html#building-with-setuptools
[2] https://google.github.io/googletest/quickstart-cmake.html

Test Plan

Add the GTest suite to the GitHub Actions workflow "Tests".

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • The title of my pull request is a short description of the requested changes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2022
@feynmanliang
Copy link
Contributor

Are we planning to refactor C++ tests as well? It makes sense to re-use Python fixtures in python tests, but I'm less clear about opportunities for re-use within BMG.

@ntfrgl
Copy link
Contributor Author

ntfrgl commented Oct 11, 2022

The Pytest fixtures are just the first, and most pressing, step of a larger refactoring effort across the BM and BMG test suites, which will increase their modularity as well as coverage of the coupling between front-end (BM) and back-ends (BM, BMG etc.). So the answer to your first question is yes, while the explanation will only unfold over a sequence of PRs which should be independently justifiable. In particular, the change proposed here was already discussed with the BMG team.

@horizon-blue
Copy link
Contributor

Thanks for the 2nd test reorganization. Do we have a plan to create an entry point to run GTest as part of this PR to enable C++ testing on external platforms?

@facebook-github-bot
Copy link
Contributor

@feynmanliang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ntfrgl
Copy link
Contributor Author

ntfrgl commented Oct 13, 2022

It should be a matter of linking against gtest_main, or, in case there are some fixtures which are currently not visible externally, adding them to a custom main() function which defines them before calling RUN_ALL_TESTS().
https://google.github.io/googletest/primer.html#invoking-the-tests

Since this change will need to be carefully compared against the internal behaviour, I'm assuming that it will be easiest for @rodrigodesalvobraz to handle. But please let me know if I can help with that.

@feynmanliang
Copy link
Contributor

@ntfrgl do you mind rebasing 😓 ?

@facebook-github-bot
Copy link
Contributor

@ntfrgl has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@feynmanliang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ndmlny-qs and others added 11 commits February 21, 2023 11:50
This PR is testing if we can merge PRs into `main`. The only thing that
has changed is the `README.md` file. **No** content was changed, only
formatting, which was done with `mdformat` with the standard line length
defined by `black`, and standardization of the markdown.
The commit modifies the GitHub actions to ensure the needed directory
for `vcpkg` exists before running the `vcpkg integrate install` command.
The `forward` method in the `Regression` class in module
`tests/ppl/experimental/gp/inference_test.py` was failing due to an
attribute error. This commit attempts to fix that.
This commit applies the same reasoning to the GTest suite for BMG/C++ as
fb7a556 (#1674) does to the Pytest suite for BM/Python. It leaves
untouched the separate source tree of `minibmg/tests/`.
- reflect test relocation in e1e1eca
- enable parallel compilation of Pybind11 extensions
- multi-version Actions: add latest release
- single-version Actions: move to oldest actively developed release
- reuse installed dependencies:
  - from Conda: Boost, Eigen, GTest
  - from Vcpkg: range-v3
- execute GTest suite via CMake's test runner
@ntfrgl ntfrgl self-assigned this Mar 5, 2023
@ntfrgl ntfrgl requested review from zaxtax and ndmlny-qs March 5, 2023 10:39
@feynmanliang
Copy link
Contributor

Hi Boyan, I'm no longer at Meta but my understanding is that beanmachine is no more. Perhaps @horizon-blue can advise?

@ntfrgl ntfrgl assigned ndmlny-qs and unassigned ntfrgl Mar 5, 2023
@horizon-blue
Copy link
Contributor

@feynmanliang We've had some discussions around the future of Bean Machine with OpenTeams. Even though the Meta team that officially supports it was gone, there's still some interests to keep BM alive for the community. So we detached BM from Meta's codebase and turned it into a GitHub-only project. Now people should be able to continue maintaining BM just like any other GitHub repo, and Meta no longer need to be involved to approve and merge PR.

@feynmanliang
Copy link
Contributor

feynmanliang commented Mar 6, 2023

Oooh.... Does beanmachine need an OSS steward (e.g. Vercel and Svelte/React)? We (Aileen ) might be able to pick it up...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants