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

Automated registration of Unit test using boost test #1811

Merged
merged 124 commits into from
Nov 8, 2023

Conversation

Sidsky
Copy link
Contributor

@Sidsky Sidsky commented Oct 12, 2023

This is first step in #1780 to update the test-suite to use automated registration. The toplevelfixture.hppis a fixture that fills in for QUANTLIB_TEST_CASE macro which is to initialise and restore settings after each test case.

The QuantLibGlobalFixture is used for a one-time setup and teardown (start/stop timer, read command line arguments, etc)

The struct if_speed is used as a predicate in precondition for selected test cases where the test cases are run based on the speed of execution.

I'm still researching on how to migrate quantlibbenchmark.cpp. Please suggest if you have any ideas!

@boring-cyborg
Copy link

boring-cyborg bot commented Oct 12, 2023

Thanks for opening this pull request! It might take a while before we look at it, so don't worry if there seems to be no feedback. We'll get to it.

@CLAassistant
Copy link

CLAassistant commented Oct 12, 2023

CLA assistant check
All committers have signed the CLA.

@lballabio
Copy link
Owner

@klausspanderen — maybe you can advise about the benchmark?

@klausspanderen
Copy link
Contributor

Hi

I think in this case we need to declare the benchmark test cases in quantlibbenchmark.cpp like

namespace QuantLibTest {
    namespace AmericanOptionTest {
        struct testFdAmericanGreeks: public BOOST_AUTO_TEST_CASE_FIXTURE { void test_method(); };
    }
}

and change the registration towards

    Benchmark("AmericanOption::FdAmericanGreeks", std::bind(&QuantLibTest::AmericanOptionTest::testFdAmericanGreeks::test_method, QuantLibTest::AmericanOptionTest::testFdAmericanGreeks()), 518.31),

and replace the typedef fct_ptr by std::function<void(void)>

@Sidsky
Copy link
Contributor Author

Sidsky commented Oct 19, 2023

I get a function undefined error in linux build. Not sure if declaring test_method() as an empty function - test_method(){}, is the correct way to resolve the issue. It does pass all the checks but fails for cmake-with-options with error C2011: 'QuantLibTest::AmericanOptionTest::testFdAmericanGreeks': 'struct' type redefinition

@Sidsky
Copy link
Contributor Author

Sidsky commented Oct 19, 2023

Hi @klausspanderen, I have updated the code with your suggested changes, however, the code fails to build on linux because of unresolved/undefined/redefinition issues. I think the reason why this happens is because the macro in americanoption.cpp-

BOOST_AUTO_TEST_CASE(testFdAmericanGreeks) {
    ...
}

replaces the code with-

struct testFdAmericanGreeks : public BOOST_AUTO_TEST_CASE_FIXTURE {
    void test_method();
}; 

.
.
.
void testFdAmericanGreeks::test_method() {
    ...
}

Since it is already defined and declared once in the americanoption.cpp file, doing it again in quantlibbenchmark.cpp causes redefinition.

If I am not mistaken, the bind function is used to bind the testFdAmericanGreeks() in the americanoption.cpp file, without defining the test code again. I'm not sure whether test_method() in quantlibbenchmark.cpp requires to add the test code again?

One possible reason for the unresolved symbol error could be that the americanoption.hpp is no longer available.

Please advise.

@klausspanderen
Copy link
Contributor

I've added the PR#5 for your repo (from my branch test_reg). Important are just the changes to the test-suite folder. Maybe the compiled americanoption.cpp file was missing in the linker step? Please let me know if it doesn't work for you.

declare benchmark test cases and add cpp to Makefile.am and CMakeLists.txt
@Sidsky
Copy link
Contributor Author

Sidsky commented Oct 20, 2023

Thanks so much! @klausspanderen

Sidsky and others added 2 commits October 20, 2023 17:19
Migrated asianoptions.cpp, array.cpp, andreasenhugevolatilityinterpl.…
@Sidsky
Copy link
Contributor Author

Sidsky commented Oct 25, 2023

Hi @lballabio @klausspanderen, please let me know if I'm moving in the right direction. We can keep this PR active, I'll keep adding more commits.

@lballabio
Copy link
Owner

Hi—yes, let's continue in this direction. Thanks!

@Sidsky
Copy link
Contributor Author

Sidsky commented Nov 7, 2023

Thanks

I'm puzzled, but I'd probably just add

set_source_files_properties(quantlibbenchmark.cpp PROPERTIES SKIP_UNITY_BUILD_INCLUSION true)

and have that file compiled on its own.

Thanks. I have added a similar fix in Makefile.am for linux build.

@@ -149,9 +150,586 @@ namespace {
f.enableExtrapolation();
return GaussLobattoIntegral(1000, 1e-6)(f, x.front(), x.back());
}

Real stationaryLogProbabilityFct(Real kappa, Real theta,
Copy link
Owner

Choose a reason for hiding this comment

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

Don't roll back these changes; but since there are still lots of files to modify, in general I'd keep helper functions closer to the tests that use them. Incidentally, this would also reduce the diffs.

Copy link
Contributor Author

@Sidsky Sidsky Nov 8, 2023

Choose a reason for hiding this comment

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

Alright, I'll try to keep them as close as possible.

You will also find that in some files I have combined segregated functions belonging to same namespace into a single namespace at the top. The reason for that is that when BOOST_FIXTURE_TEST_SUITE and BOOST_AUTO_TEST_SUITE is declared, it creates a new namespace. If a namespace is declared before the fixture declaration- say namespace a, that would act as a top-level namespace, however, if the same namespace is used to add more function in it between test cases, that declares a new namespace which is FixtureName::SuiteName::a. To avoid conflicts and namespace errors I choose to restructure them and combine them into one before the fixture declaration.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I see. In that case, you're right, do move them at the top.

@lballabio
Copy link
Owner

So, should we merge what we have so far and continue in another PR? Or do you want to continue here?

@Sidsky
Copy link
Contributor Author

Sidsky commented Nov 8, 2023

yes, I think we can start merging. I'll finish migrating the remaining files in the next PR

@lballabio lballabio added this to the Release 1.33 milestone Nov 8, 2023
@lballabio
Copy link
Owner

Ok, thanks. Later today I'll push a further fix to the check_test_times script and then I'll merge the PR.

@lballabio lballabio merged commit 77418dc into lballabio:master Nov 8, 2023
49 checks passed
Copy link

boring-cyborg bot commented Nov 8, 2023

Congratulations on your first merged pull request!

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.

5 participants