-
Notifications
You must be signed in to change notification settings - Fork 1
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
Initial pass at implementing test fixtures #316
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
std::vector<double> factorsMale = {200.,40.,30.,2000.,1.,1000.,3.,50.}; | ||
std::vector<double> factorsFemale = {200.,40.,30.,2000.,1.,1000.,3.,50.}; | ||
|
||
std::unordered_map<hgps::core::Identifier, std::vector<double>> factorsMale = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: redefinition of 'factorsMale' with a different type: 'std::unordered_map<hgps::core::Identifier, std::vector>' vs 'std::vector' [clang-diagnostic-error]
std::unordered_map<hgps::core::Identifier, std::vector<double>> factorsMale = {
^
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:6: previous definition is here
std::vector<double> factorsMale = {200.,40.,30.,2000.,1.,1000.,3.,50.};
^
{hgps::core::Identifier{"1"}, factorsMale} | ||
}; | ||
|
||
std::unordered_map<hgps::core::Identifier, std::vector<double>> factorsFemale = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: redefinition of 'factorsFemale' with a different type: 'std::unordered_map<hgps::core::Identifier, std::vector>' vs 'std::vector' [clang-diagnostic-error]
std::unordered_map<hgps::core::Identifier, std::vector<double>> factorsFemale = {
^
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:7: previous definition is here
std::vector<double> factorsFemale = {200.,40.,30.,2000.,1.,1000.,3.,50.};
^
{hgps::core::Identifier{"1"}, factorsFemale} | ||
}; | ||
|
||
expected.emplace_row(hgps::core::Gender::male, factorsMale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'expected' [clang-diagnostic-error]
expected.emplace_row(hgps::core::Gender::male, factorsMale);
^
{hgps::core::Identifier{"1"}, factorsFemale} | ||
}; | ||
|
||
expected.emplace_row(hgps::core::Gender::male, factorsMale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: cannot use dot operator on a type [clang-diagnostic-error]
expected.emplace_row(hgps::core::Gender::male, factorsMale);
^
}; | ||
|
||
expected.emplace_row(hgps::core::Gender::male, factorsMale); | ||
expected.emplace_row(hgps::core::Gender::female, factorsFemale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'expected' [clang-diagnostic-error]
expected.emplace_row(hgps::core::Gender::female, factorsFemale);
^
{"Sector", -0.0629531974852436}, | ||
{"Income", 0.373256996730791} | ||
}; | ||
fat_model.coefficients = coefficients; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: cannot use dot operator on a type [clang-diagnostic-error]
fat_model.coefficients = coefficients;
^
}; | ||
fat_model.coefficients = coefficients; | ||
|
||
models.emplace_back(std::move(carb_model)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'models'; did you mean 'mode_t'? [clang-diagnostic-error]
models.emplace_back(std::move(carb_model)); | |
mode_t.emplace_back(std::move(carb_model)); |
Additional context
/usr/include/x86_64-linux-gnu/sys/types.h:68: 'mode_t' declared here
typedef __mode_t mode_t;
^
}; | ||
fat_model.coefficients = coefficients; | ||
|
||
models.emplace_back(std::move(carb_model)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: cannot use dot operator on a type [clang-diagnostic-error]
models.emplace_back(std::move(carb_model));
^
fat_model.coefficients = coefficients; | ||
|
||
models.emplace_back(std::move(carb_model)); | ||
models.emplace_back(std::move(fat_model)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'models'; did you mean 'mode_t'? [clang-diagnostic-error]
models.emplace_back(std::move(fat_model)); | |
mode_t.emplace_back(std::move(fat_model)); |
Additional context
/usr/include/x86_64-linux-gnu/sys/types.h:68: 'mode_t' declared here
typedef __mode_t mode_t;
^
fat_model.coefficients = coefficients; | ||
|
||
models.emplace_back(std::move(carb_model)); | ||
models.emplace_back(std::move(fat_model)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: cannot use dot operator on a type [clang-diagnostic-error]
models.emplace_back(std::move(fat_model));
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
std::vector<double> factorsMale = {200.,40.,30.,2000.,1.,1000.,3.,50.}; | ||
std::vector<double> factorsFemale = {200.,40.,30.,2000.,1.,1000.,3.,50.}; | ||
|
||
std::unordered_map<hgps::core::Identifier, std::vector<double>> factorsMale = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: redefinition of 'factorsMale' with a different type: 'std::unordered_map<hgps::core::Identifier, std::vector>' vs 'std::vector' [clang-diagnostic-error]
std::unordered_map<hgps::core::Identifier, std::vector<double>> factorsMale = {
^
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:20: previous definition is here
std::vector<double> factorsMale = {200.,40.,30.,2000.,1.,1000.,3.,50.};
^
{hgps::core::Identifier{"1"}, factorsMale} | ||
}; | ||
|
||
std::unordered_map<hgps::core::Identifier, std::vector<double>> factorsFemale = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: redefinition of 'factorsFemale' with a different type: 'std::unordered_map<hgps::core::Identifier, std::vector>' vs 'std::vector' [clang-diagnostic-error]
std::unordered_map<hgps::core::Identifier, std::vector<double>> factorsFemale = {
^
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:21: previous definition is here
std::vector<double> factorsFemale = {200.,40.,30.,2000.,1.,1000.,3.,50.};
^
{"FoodSodium", -0.0396319735508116} | ||
}; | ||
carb_policy_model.log_coefficients = log_coefficients; | ||
policy_ranges.emplace_back(hgps::core::DoubleInterval{-2.31063, 0.37526}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unnecessary temporary object created while calling emplace_back [modernize-use-emplace]
policy_ranges.emplace_back(hgps::core::DoubleInterval{-2.31063, 0.37526}); | |
policy_ranges.emplace_back(-2.31063, 0.37526); |
{"Age", 0.000557687923688474}, | ||
{"Sector", 0.00166651997223223}, | ||
{"Income", -0.498755023022772} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: expected ';' after expression [clang-diagnostic-error]
} | |
}; |
{"FoodSodium", -0.17181077457271} | ||
}; | ||
fat_policy_model.log_coefficients = log_coefficients; | ||
policy_ranges.emplace_back(hgps::core::DoubleInterval{-4.87205, -0.10346}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unnecessary temporary object created while calling emplace_back [modernize-use-emplace]
policy_ranges.emplace_back(hgps::core::DoubleInterval{-4.87205, -0.10346}); | |
policy_ranges.emplace_back(-4.87205, -0.10346); |
cholesky, | ||
policy_models, | ||
policy_ranges, | ||
policy_cholesky, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'policy_cholesky' [clang-diagnostic-error]
policy_cholesky,
^
policy_models, | ||
policy_ranges, | ||
policy_cholesky, | ||
info_speed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'info_speed' [clang-diagnostic-error]
info_speed,
^
policy_ranges, | ||
policy_cholesky, | ||
info_speed, | ||
rural_prevalence, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'rural_prevalence' [clang-diagnostic-error]
rural_prevalence,
^
policy_cholesky, | ||
info_speed, | ||
rural_prevalence, | ||
income_models, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'income_models' [clang-diagnostic-error]
income_models,
^
info_speed, | ||
rural_prevalence, | ||
income_models, | ||
physical_activity_stddev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'physical_activity_stddev' [clang-diagnostic-error]
physical_activity_stddev
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
// Create test fixture for a StaticLinearModel class instance | ||
class StaticLinearModelTestFixture : public::testing::Test { | ||
public: | ||
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member initializer 'params_' does not name a non-static data member or base class [clang-diagnostic-error]
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) { | ||
// Create a StaticLinearModel instance | ||
hgps::StaticLinearModel testModel{ | ||
params_.expected, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.expected, | |
params.expected, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:166: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
// Create a StaticLinearModel instance | ||
hgps::StaticLinearModel testModel{ | ||
params_.expected, | ||
params_.names, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.names, | |
params.names, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:166: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
hgps::StaticLinearModel testModel{ | ||
params_.expected, | ||
params_.names, | ||
params_.models, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.models, | |
params.models, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:166: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
params_.expected, | ||
params_.names, | ||
params_.models, | ||
params_.lambda, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.lambda, | |
params.lambda, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:166: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
params_.cholesky, | ||
params_.policy_models, | ||
params_.policy_ranges, | ||
params_.policy_cholesky, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.policy_cholesky, | |
params.policy_cholesky, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:166: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
params_.policy_models, | ||
params_.policy_ranges, | ||
params_.policy_cholesky, | ||
params_.info_speed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_' [clang-diagnostic-error]
params_.info_speed,
^
params_.policy_ranges, | ||
params_.policy_cholesky, | ||
params_.info_speed, | ||
params_.rural_prevalence, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_' [clang-diagnostic-error]
params_.rural_prevalence,
^
params_.policy_cholesky, | ||
params_.info_speed, | ||
params_.rural_prevalence, | ||
params_.income_models, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_' [clang-diagnostic-error]
params_.income_models,
^
params_.info_speed, | ||
params_.rural_prevalence, | ||
params_.income_models, | ||
params_.physical_activity_stddev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_' [clang-diagnostic-error]
params_.physical_activity_stddev
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TinyMarsh. Looks like it's taking shape!
Some minor notes about initialisation, name convention and model name, but on the right track 👍
Happy to take another look later on.
Sorry, just saw the fixture class. I think what you'll find is that you need to initialise all the vectors and tables and stuff above inside the fixture constructor, above where you construct the staticlinearmodel, as I don't think there's a way to pass in arguments to fixtures. You'll also need a variable inside the fixture class to actually store the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
policy_models.emplace_back(std::move(carb_policy_model)); | ||
policy_models.emplace_back(std::move(fat_policy_model)); | ||
|
||
const double info_speed = 0.1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unused variable 'info_speed' [clang-diagnostic-unused-variable]
const double info_speed = 0.1;
^
income_models.emplace(hgps::core::Income::middle, std::move(income_model_middle)); | ||
income_models.emplace(hgps::core::Income::high, std::move(income_model_high)); | ||
|
||
const double physical_activity_stddev = 0.06; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unused variable 'physical_activity_stddev' [clang-diagnostic-unused-variable]
const double physical_activity_stddev = 0.06;
^
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) { | ||
// Create a StaticLinearModel instance | ||
hgps::StaticLinearModel testModel{ | ||
params_.expected, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.expected, | |
params.expected, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:157: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
// Create a StaticLinearModel instance | ||
hgps::StaticLinearModel testModel{ | ||
params_.expected, | ||
params_.names, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.names, | |
params.names, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:157: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
hgps::StaticLinearModel testModel{ | ||
params_.expected, | ||
params_.names, | ||
params_.models, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.models, | |
params.models, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:157: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
params_.names, | ||
params_.models, | ||
params_.lambda, | ||
params_.stddev, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.stddev, | |
params.stddev, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:157: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
params_.models, | ||
params_.lambda, | ||
params_.stddev, | ||
params_.cholesky, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.cholesky, | |
params.cholesky, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:157: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
params_.lambda, | ||
params_.stddev, | ||
params_.cholesky, | ||
params_.policy_models, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.policy_models, | |
params.policy_models, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:157: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
params_.stddev, | ||
params_.cholesky, | ||
params_.policy_models, | ||
params_.policy_ranges, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.policy_ranges, | |
params.policy_ranges, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:157: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
params_.cholesky, | ||
params_.policy_models, | ||
params_.policy_ranges, | ||
params_.policy_cholesky, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use of undeclared identifier 'params_'; did you mean 'params'? [clang-diagnostic-error]
params_.policy_cholesky, | |
params.policy_cholesky, |
Additional context
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:157: 'params' declared here
StaticLinearModelTestFixture(const ModelParameters& params) : params_(params) {
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
#include "HealthGPS/static_linear_model.h" | ||
|
||
// Create test fixture for a StaticLinearModel class instance | ||
class StaticLinearModelTestFixture : public::testing::Test { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: constructor does not initialize these fields: testModel [cppcoreguidelines-pro-type-member-init]
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:19:
- StaticLinearModel* testModel;
+ StaticLinearModel* testModel{};
std::vector<hgps::core::DoubleInterval> policy_ranges; | ||
Eigen::MatrixXd policy_cholesky; | ||
|
||
StaticLinearModel* testModel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown type name 'StaticLinearModel'; did you mean 'hgps::StaticLinearModel'? [clang-diagnostic-error]
StaticLinearModel* testModel; | |
hgps::StaticLinearModel* testModel; |
Additional context
src/HealthGPS/static_linear_model.h:22: 'hgps::StaticLinearModel' declared here
class StaticLinearModel final : public RiskFactorAdjustableModel {
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
#include "HealthGPS/static_linear_model.h" | ||
|
||
// Create test fixture for a StaticLinearModel class instance | ||
class StaticLinearModelTestFixture : public::testing::Test { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: constructor does not initialize these fields: testModel, testPerson, testRandom [cppcoreguidelines-pro-type-member-init]
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:19:
- hgps::StaticLinearModel* testModel;
- hgps::Person* testPerson;
- hgps::Random* testRandom;
+ hgps::StaticLinearModel* testModel{};
+ hgps::Person* testPerson{};
+ hgps::Random* testRandom{};
|
||
const double physical_activity_stddev = 0.06; | ||
|
||
testPerson = new hgps::Person(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: assigning newly created 'gsl::owner<>' to non-owner 'hgps::Person *' [cppcoreguidelines-owning-memory]
testPerson = new hgps::Person();
^
const double physical_activity_stddev = 0.06; | ||
|
||
testPerson = new hgps::Person(); | ||
testRandom = new hgps::Random(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: call to deleted constructor of 'hgps::Random' [clang-diagnostic-error]
testRandom = new hgps::Random();
^
Additional context
src/HealthGPS/random_algorithm.h:9: 'Random' has been explicitly marked deleted here
Random() = delete;
^
testPerson = new hgps::Person(); | ||
testRandom = new hgps::Random(); | ||
|
||
testModel = new hgps::StaticLinearModel(names, models, lambda, stddev, cholesky, policy_models, policy_ranges, policy_cholesky, info_speed, rural_prevalence, income_models, physical_activity_stddev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no matching constructor for initialization of 'hgps::StaticLinearModel' [clang-diagnostic-error]
testModel = new hgps::StaticLinearModel(names, models, lambda, stddev, cholesky, policy_models, policy_ranges, policy_cholesky, info_speed, rural_prevalence, income_models, physical_activity_stddev);
^
Additional context
src/HealthGPS/static_linear_model.h:39: candidate constructor not viable: requires 13 arguments, but 12 were provided
StaticLinearModel(
^
src/HealthGPS/static_linear_model.h:22: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 12 were provided
class StaticLinearModel final : public RiskFactorAdjustableModel {
^
src/HealthGPS/static_linear_model.h:22: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 12 were provided
class StaticLinearModel final : public RiskFactorAdjustableModel {
^
|
||
TEST_F(StaticLinearModelTestFixture, InitialiseFactors) { | ||
// Just check that initialise_factors runs successfully | ||
testModel->initialise_factors(*testPerson, *testRandom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'initialise_factors' is a private member of 'hgps::StaticLinearModel' [clang-diagnostic-error]
testModel->initialise_factors(*testPerson, *testRandom);
^
Additional context
src/HealthGPS/static_linear_model.h:62: declared private here
void initialise_factors(Person &person, Random &random) const;
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
#include "HealthGPS/mtrandom.h" | ||
|
||
// Create test fixture for a StaticLinearModel class instance | ||
class StaticLinearModelTestFixture : public::testing::Test { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: constructor does not initialize these fields: test_model, test_person, test_random [cppcoreguidelines-pro-type-member-init]
src/HealthGPS.Tests/StaticLinearModel.Test.cpp:20:
- hgps::StaticLinearModel* test_model;
- hgps::Person* test_person;
- hgps::Random* test_random;
+ hgps::StaticLinearModel* test_model{};
+ hgps::Person* test_person{};
+ hgps::Random* test_random{};
|
||
const double physical_activity_stddev = 0.06; | ||
|
||
test_person = new hgps::Person(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: assigning newly created 'gsl::owner<>' to non-owner 'hgps::Person *' [cppcoreguidelines-owning-memory]
test_person = new hgps::Person();
^
|
||
test_person = new hgps::Person(); | ||
auto engine = hgps::MTRandom32{123456789}; | ||
test_random = new hgps::Random(engine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: assigning newly created 'gsl::owner<>' to non-owner 'hgps::Random *' [cppcoreguidelines-owning-memory]
test_random = new hgps::Random(engine);
^
auto engine = hgps::MTRandom32{123456789}; | ||
test_random = new hgps::Random(engine); | ||
|
||
test_model = new hgps::StaticLinearModel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: assigning newly created 'gsl::owner<>' to non-owner 'hgps::StaticLinearModel *' [cppcoreguidelines-owning-memory]
test_model = new hgps::StaticLinearModel(
^
|
||
TEST_F(StaticLinearModelTestFixture, InitialiseFactors) { | ||
// Just check that initialise_factors runs successfully | ||
test_model->initialise_factors(*test_person, *test_random); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'initialise_factors' is a private member of 'hgps::StaticLinearModel' [clang-diagnostic-error]
test_model->initialise_factors(*test_person, *test_random);
^
Additional context
src/HealthGPS/static_linear_model.h:62: declared private here
void initialise_factors(Person &person, Random &random) const;
^
Hi @jamesturner246, I think most of the structure is in place now to fill out the actual logic for testing the Since we want to be testing code that is in private member functions, what do you think is the nicest way to do that? GoogleTest documentation suggests it isn't nice at all and that the design of the class needs a rethink. We're a bit beyond that so I was looking at the suggestions, it seems that |
Looking good so far. I think the A couple of points:
Thanks! |
Some of the models/policy models have been implemented. I am having difficulty with how to instantiate the correlation matrices so would appreciate some help there.
Many of the parameters are remaining as a todo. Addresses #289