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

Missing and failing C++ tests #1864

Open
ahoenselaar opened this issue Dec 23, 2021 · 2 comments
Open

Missing and failing C++ tests #1864

ahoenselaar opened this issue Dec 23, 2021 · 2 comments

Comments

@ahoenselaar
Copy link
Contributor

The current check testsuite only runs 22 C++ tests (20 when MPI is enabled) because eight tests are missing from the TESTS Automake variable:
https://github.com/NanoComp/meep/blob/master/tests/Makefile.am#L113

Out of the eight missing tests, four are failing at HEAD in a double-precision build:

  • array-metadata
  • dft-fields
  • gdsII-3d
  • ring-ll

In addition, absorber-1d-ll fails in single-precision builds.

@oskooi
Copy link
Collaborator

oskooi commented Jan 3, 2022

For reference, see #1878 which adds one of these missing tests (ring-ll.cpp) to the C++ make check suite. As I mention in #1878, there is probably no need to include every single missing C++ test since, given the large number and variety of existing C++ and Python tests, it is unlikely any of these missing tests will increase coverage. For example, there already exists two Python tests which involve the GDS import feature (test_bend_flux.py and test_prism.py). Also, gdsII-3d.cpp (one of the missing tests) has bit rotted and would need to be overhauled.

(Unfortunately, the Codecov service seems to be currently malfunctioning because the report for #1878 states that including ring-ll.cpp will reduce coverage by ~23% which is obviously incorrect.)

@stevengj
Copy link
Collaborator

stevengj commented Jan 5, 2022

I think these were added when meepgeom.cpp was created, before meepgeom was used by the Python API. Now that the Python API uses meepgeom.cpp, I think these tests are redundant?

I would be open to just deleting them, if someone goes through and double-checks that they are exercising functions that are also used by the Python API.

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

No branches or pull requests

3 participants