tests for rule.cc (experiment)

[wandadars]

There should be 5 failing tests at the top of the test file for rule.cc

[EdwardALuke]

Ok, I am assuming that this was an AI generated suite of tests. Some of these errors are related to old code that is no longer relevant to current usage of the rule API, in particular the names assigned to rules were there for the purpose of managing manually generated schedules (way back in the origin of the Loci more than 25 years ago). There are some other errors related to the removal of rules from the rule database not managing some secondary data structures consistently. This is not an issue with the present usage of this feature so not a high priority fix. Looking through some of the rules, I am also seeing nonsensical tests, such as a test that a duplicate rule was not added to ruleSet a second time by counting the rules in the ruleSet, but the ruleSet can't contain a duplicate rule so this test seems to be a non-sequiturs. At any rate, this test would be more appropriate for the ruleSet instead of the rule database.

Assuming this is automatically generated tests, then it is interesting and useful, but probably needs to be applied judiciously. I haven't looked over all of the tests yet, but I am concerned that some of the tests my be testing parts of the API that are intended for internal use and might change with implementation. In that case, having tests that include these could make future code refactoring more work. Probably in that case there internal API should be protected with private or similar C++ features. I haven't had a chance to look at every test but there may be some of these sorts of issues here as well. I know that I have been planning to refactor both the variable and rule infrastructure to improve performance of dependency graph generation. Right now there is excessive reliance on string processing to do operations associated with rule promotion, etc. So I will be concerned about tests that test things that aren't part of the functional spec (e.g. the behavior that we need to rely on, v.s. implementation details.) Probably this would be a good starting point for creating appropriate tests, but it should probably be part of a refactoring exercise. For example, if you are generating tests using methods that are not called in the code, then maybe that is evidence that this is a feature or function that should be removed from the API rather than tested.

To that end I have created a branch called rule-begin-refactor that addresses some of these issues. Your tests will need to remove tests that are calling parts of the API that are removed. I don't wan't to merge this into the 4.1 version as I think this is the type of change that might impact stability. As I said, I think that this type of activity is probably going to have to go hand-in-hand with some sort of code refactor. We can address adding tests of this nature after we have branched off a 4.1 stable branch.

[wandadars]

Thanks Ed. I did not know about the old API. That definitely explains those failing tests. Is there a large API surface of older things that aren't used? I know the Cantera project uses something like a special function that they put inside elements that are marked for deprecation that prints things like "this is schedule for deprecation in version X.X". And that lets them search and clear those things out after a release. If the API is small, then nothing like that would be necessary.

I agree that we can just leave this here for now. I'll check that branch out and clear out any of those irrelevant tests.

[EdwardALuke]

It is not like we have a design document that lays out all of the parts. Instead this is a research project and as the code evolves some parts fall into disuse. Usually when I refactor I will look at how the API is being used and eliminate extra cruft. There are many parts of the system that you are looking at that are only intended to be part of the internal scheduling infrastructure and thus are only usually visible to a select few developers. I generally don't worry about some sort of deprecation schedule for these because they are calls that shouldn't be made by user code. Usually when I have a stable release I consider doing some code refactoring as an early step for the next release. So for example, when we move on to the 4.2 version one of the first things that I am going to do is clean up or eliminate some of these internal API's that should have no impact on end users.

In this specific case in the very early days of the code development I had names for rules to make it simple to manually generate schedules. But once we had automatic scheduling of rules this feature was no longer used. It really caused no problems that it was in there because it wasn't represented in the example codes or tutorial that everyone is using as defacto documentation. So it isn't a high priority to remove it, and typically there is too little time to make everything perfect. Instead the software artifacts that are actually causing software faults is where the time and energy gets spent. That can be an issue for automatic test generation, especially if tests are based on the code and not some documentation of what the API is supposed to be. If you are using part of the API that you don't see a use case in any of the existing application examples, then you are probably in dangerous territory. The behavior of these parts may be incomplete implementations of features that are not yet working, or they may be unsupported ancient code, and they may be removed anytime that a non-patch version change happens (and sometimes maybe in a patch revision change if this really is some buried part of the internal interface.)

That is why I suggest that making AI generated tests probably needs to be coupled with an intentional plan for code refactoring. In many cases the tests may be checking things that are better removed than fixed. This requires understanding the history and future plans for the code. But it can be helpful: if you asked me about the rule name infrastructure before this pull request, I wouldn't have remembered that it was even in there, so it wouldn't have been on my radar to remove it.

[wandadars]

Cleared out some of those calls that were being tested despite being state/unused from your branch. Some other ones that perhaps are deprecated/unused in the rule.cc file, in the sense that they don't seem to have callers:

rule::add_namespace
void rule_impl::set_variable_times(time_ident tl)
rules_by_source
rule_impl::split_constraints

[wandadars]

Added the ability to execute particular tests from the top level. Also moved the doctest.h out of that include/ under Containers/ and into the contrib/, which I believe is like a third-party folder so that there's a clearer tracking of this dependency.

[wandadars]

Also marked those tests that are failing with the doctest may_fail() so that they don't hard stop the test suite.

[wandadars]

I'm thinking that if any of this stuff is worthwhile, we'd focus on this PR first, and then all the other ones would just rebase onto this one to leverage the updates to the testing framework. The goal would be to have tests for source code on the critical path for Loci to try and get the coverage up for the code. And while looking at the source files, try to add any Doxygen style docstrings .

[EdwardALuke]

Ok, I am beginning refactoring exercise on Loci. First I would like these pull requests to be more narrowly focused. For example, this should just be focused on tests for rule and rule_db, not include documentation of rule.cc and rule.h. So excise those changes. Also, doctest,h shouldn't be moved all the way out to contrib. I see contrib as being optional tools that can either be installed prior to installing Loci or installed afterwards. But the doctest is integral. For modularity it should be in the quickTest directory. We could add a contrib directory under quickTest to segregate it out.

Basically I don't want to have Makefiles that refer to the SOURCE_DIR because this breaks modularity. Ideally I would like the quickTest to be something that could be run against any installed version of Loci, but if you are expecting sources (that typically aren't installed) for the tests to work, then quickTest can't be used in this modular way. I have built all of Loci with this type of modular facility in mind. The OBJ directory represents the installed version of Loci while building. (I always use the OBJ directory this way during development, e.g. when I compile chem I use the OBJ directory as LOCI_BASE). Similarly I often will work with lpp or other submodules against an installed version of Loci. So this philosophy has been used throughout the Loci build system. The parts are built as independent modules that can be treated separately if it is desirable to do so. So in that philosophy, we would want to keep the doctest under the quickTest module, not move it someplace else in the source code structure. And as a general rule, I don't want to explicitly reference the SOURCE_DIR as this assumes we have the sources available, which we don't when using something as a module compiled using an installed version of the software.

[wandadars]

Thanks @EdwardALuke . I will split up the documentation/tests. I get sidetracked easily. And I'll revert that doctest change. I have about 6 other branches now that are narrowly focused on trying to get coverage of the existing source, with the goal of filling out the test suite along a critical path.

[EdwardALuke]

Hold of on making any changes until the recent changes to the quickTest have been merged in. I am changing the infrastructure there to improve modularity. The new version also collates the test results and shows the failed tests as the last output before failure. I have generalized the doctest based tests so that there is one Makefile can be reused for all of these tests.

[EdwardALuke]

Ok, the latest dev has implemented the changes to the quickTest infrastructure and rule refactor. Edit this PR to just include the rule tests. Copy the make file from the Containers or Tools directory and it will add the tests using the new framework.

[wandadars]

Thanks @EdwardALuke . I pulled all the extraneous stuff out of this branch. It should be ready now.