search

openshmem-org / tests-sos

Sandia OpenSHMEM unit tests and performance testing suite
Other
6 stars 11 forks source link

Add standard cxx tests #6

Closed minsii closed 5 years ago

minsii commented 5 years ago

This PR moves existing CXX tests that uses SOS-specific CXX generic APIs to test/shmemx, and adds CXX tests with standard CXX APIs in test/unit.

minsii commented 5 years ago

@jdinan We have tested the PR with OSHMPI. Could you please review it for me ?

jdinan commented 5 years ago

Thanks for the PR! I am on vacation this week; I'll take a look first thing on Monday.

jdinan commented 5 years ago

@minsii Apologies; we will need to merge the changes from #9 into this PR. This is a consequence of tests-sos lagging behind the upstream sos repository. Fortunately, your branch has only one trivial conflict rebasing onto the new master. We have a few options for how to proceed (1) you can do the rebase and force push (2) I can do the rebase and force push to your branch (assuming you allowed this when you posted the PR) or (3) I can post a new PR using my branch. How would you like to proceed?

jdinan commented 5 years ago

Do we want to also test the non-generic NBI AMOs in the test/unit/cxx_* tests?

jdinan commented 5 years ago

Tests pass when merged into upstream SOS repository.

minsii commented 5 years ago

Thanks @jdinan . We do not have NBI AMOs in spec 1.4. So I'd suggest put them as shmemx for now, and we can move them back to test/unit once spec 1.5 is out.

I can work on the merge+force push later tonight. I guess you can also force push to my fork (I am not sure if it works...)

jdinan commented 5 years ago

@minsii In this case, I think it may be easier to rebase on master and force push, rather than trying to merge and do a non-forced push. The NBI test cases are actually there already (but gated by ENABLE_SHMEMX_TESTS) in SOS and your PR is removing them. We would like to account for these APIs in our generic bindings tests (C11 and CXX). Given that these exercise all of the bindings in 1.4, we don't want to move them to tests/shmemx which is why we enable/disable the NBI AMO cases with ENABLE_SHMEMX_TESTS.

minsii commented 5 years ago

@jdinan : Sorry for the late update. I just checked the diff with tests-sos master to better understand the issue about NBI AMO tests. I am not very sure if the following change to this PR works for you. Could you please confirm ?

jdinan commented 5 years ago

Sure, I think the changes are good. It looks like the version of the CXX tests you started with was a bit behind the tests in SOS, so the NBI tests weren't accounted for. I posted #10 to show you what I mean.

minsii commented 5 years ago

@jdinan I added non-generic NBI AMO in test/unit. Could you please review ?

I noticed that we still use the ENABLE_SHMEMX_TESTS macro for test/shmemx/cxx_* tests. Although this minimizes the diff with test/unit/c11_*, the macro seems unnecessary for shmemx tests. Do you think we can delete it ?

minsii commented 5 years ago

Great :-)