Keep only important integration tests: step 1

[mgovers]

• move some integration tests from test_main_model*.cpp to validation cases
• remove integration tests from test_main_model*.cpp that are already captured elsewhere

relates to #486

[figueroa1395]

This review is for the first commit only, but it looks like the rest of the commits do what is explained on the first, so I will review the rest by leaving individual comments.

[TonyXiang8787]

@mgovers my general idea is to remove all test_main_model, test_main_model_sc and test_main_model_se.

[mgovers]

@mgovers my general idea is to remove all test_main_model, test_main_model_sc and test_main_model_se.

yes, i know. i'm rewriting some of them, but it's a time-consuming process. all TODOs are mainly there for my personal reference, and in addition, those were all added before our discussion last thursday. I just have not updated them yet because I've been focusing on actually doing the refactoring; starting with test_main_model.cpp.

Notice that there are some that i marked "we should keep this" or "we should whitebox test that". I will make API tests for those for now, but they should not be. they point at the deeper, underlying issue of the fact that the code is very hard to test in an isolated environment. A good example is the set of tests regarding the batch dispatch logic. they should not live in API model, nor in integration tests. They should be unit tests that only test the batch dispatch logic. However, with the current architecture, it is not possible to re-write the tests in such a way and definitely out of scope of this PR.

[figueroa1395]

@mgovers my general idea is to remove all test_main_model, test_main_model_sc and test_main_model_se.

yes, i know. i'm rewriting some of them, but it's a time-consuming process. all TODOs are mainly there for my personal reference, and in addition, those were all added before our discussion last thursday. I just have not updated them yet because I've been focusing on actually doing the refactoring; starting with test_main_model.cpp.

Notice that there are some that i marked "we should keep this" or "we should whitebox test that". I will make API tests for those for now, but they should not be. they point at the deeper, underlying issue of the fact that the code is very hard to test in an isolated environment. A good example is the set of tests regarding the batch dispatch logic. they should not live in API model, nor in integration tests. They should be unit tests that only test the batch dispatch logic. However, with the current architecture, it is not possible to re-write the tests in such a way and definitely out of scope of this PR.

Out of scope, but do we also want to just move the mentioned tests to the API? Isn't that moving the problem around while using your time on it? If some are being moved to the API tests just for the sake of moving them out of integration rather than because they belong there, I would say that we just leave them be here. Just my opinion.

[mgovers]

@mgovers my general idea is to remove all test_main_model, test_main_model_sc and test_main_model_se.

yes, i know. i'm rewriting some of them, but it's a time-consuming process. all TODOs are mainly there for my personal reference, and in addition, those were all added before our discussion last thursday. I just have not updated them yet because I've been focusing on actually doing the refactoring; starting with test_main_model.cpp.
Notice that there are some that i marked "we should keep this" or "we should whitebox test that". I will make API tests for those for now, but they should not be. they point at the deeper, underlying issue of the fact that the code is very hard to test in an isolated environment. A good example is the set of tests regarding the batch dispatch logic. they should not live in API model, nor in integration tests. They should be unit tests that only test the batch dispatch logic. However, with the current architecture, it is not possible to re-write the tests in such a way and definitely out of scope of this PR.

Out of scope, but do we also want to just move the mentioned tests to the API? Isn't that moving the problem around while using your time on it? If some are being moved to the API tests just for the sake of moving them out of integration rather than because they belong there, I would say that we just leave them be here. Just my opinion.

i think so as well, but compilation time is a real issue. it's probably a good idea to re-evaluate those when the rest of the (re)moving is finished

[sonarqubecloud]

Quality Gate passed

Issues
44 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud