ILIAS-eLearning
Test & Assessment: Enhance unit tests and expand coverage for improved code quality
This PR sums up the collective work of @malaja11, @bidzanaaron, @thojou and @matheuszych on expanding the unit test coverage of the Test & Assessment component. Our initiative was not motivated by any feature wiki request or mantis issue but rather by the will to improve the overall quality of the Test & Assessment code base and increase our knowledge base about ILIAS Core.
We not only added many new tests and test cases but also improved already existing ones. While doing that we also refactored some of source code that we came across.
To increase the efficiency and quality of our unit test development, we built some helper tools that helped us to accomplish just that.
It seems this PR modifies code outside the "T&A" component -> "Administration". Please revert these changes and focus on the "T&A" only.
Hey @kergomard,
Thank you so much for your detailed feedback. We took the time to discuss it, and @matheuszych has already started implementing some changes. We've realized that some of our adjustments were overly aggressive, and we’ll be reversing them accordingly.
I would suggest ordering use statements by their source, grouping Test-related uses, then QuestionPool, followed by other ILIAS uses, and finally dependencies. If you agree, we could implement this when we touch the use statements.
We definitely agree that keeping use statements organized is important, but introducing custom code conventions such as grouping by context might create unnecessary overhead and be prone to errors, especially when auto-imports or IDE hotkeys handle these tasks. A more efficient approach could be adding the ordered_imports rule to php-cs-fixer, ensuring this is managed automatically.
The naming of test functions. I think we should be precise about what the function actually tests, so test_Getter isn't ideal...
Agreed. Improving test function naming for better clarity and ensuring each test has a clear purpose is a good practice. We’ll work on this going forward.
Personally, I also started thinking it might be a good idea to move tests to their own sub-namespace...
We completely agree! Organizing tests into their own namespace like ILIAS\Test\Tests makes sense. But we think this should be done in a separate PR.
Lastly, we should discuss what we want to test and how... many proposed tests here aren't very helpful.
You make a valid point. It’s crucial that test cases reflect real-world scenarios and provide meaningful validation. After reviewing the CertificateTestObjectHelper, we found that the class itself didn’t offer much value—it was essentially just a wrapper with no real functionality. For this reason, instead of writing more meaningful tests, we’ve decided to remove both the class and its tests. That said, we still believe smaller tests, like those for getters and setters, can be useful in preventing simple mistakes like typos.
Regarding your specific requests:
Please remove all use statements for classes in the global space.
We believe that including global namespace classes in the use statements makes the dependency list more complete. We don’t have a strong stance on this, but we’d appreciate further insight if we're breaking any conventions here.
Please don't add @throws doc-blocks.
Could you clarify why @throws tags are discouraged? They help annotate methods that might throw exceptions, making it easier to identify where custom error handling is needed, especially within an IDE.
Thank you again for your time and thoughtful feedback. We look forward to discussing this further at the next TechSquad meeting.
Best regards, @thojou
Hi @thojou
Thanks for taking this up!
I would suggest ordering use statements by their source, grouping Test-related uses, then QuestionPool, followed by other ILIAS uses, and finally dependencies. If you agree, we could implement this when we touch the use statements.
We definitely agree that keeping use statements organized is important, but introducing custom code conventions such as grouping by context might create unnecessary overhead and be prone to errors, especially when auto-imports or IDE hotkeys handle these tasks. A more efficient approach could be adding the ordered_imports rule to php-cs-fixer, ensuring this is managed automatically.
This creates overhead, yes, but it also forces us to keep our headers clean (the amount of unused use-statements I've seen just because we rely on IDEs to add them is a sign for me, that it is good to check them from time to time). I don't think that we can delegate this to any automation, as this is about semantics (not syntax) and automation (as the cs-fixer as I understand it) operate on syntax (it just orders them alphabetically or by length). Additionally the cs-fixer rule would be for everyone and I don't think we are going to agree on that for all components. So, I would be in favor to clean this up manually, whenever one looks at them and if they are not perfect it doesn't matter too much, it is just for a little bit more structure.
On the additional points:
Please remove all use statements for classes in the global space.
We believe that including global namespace classes in the use statements makes the dependency list more complete. We don’t have a strong stance on this, but we’d appreciate further insight if we're breaking any conventions here.
Two answers to this:
- I think you are the only ones (one of very few) I've seen doing this in the ILIAS code, so this would be kind of a convention, but I think this is the lesser argument.
- More importantly: Everything we do, as I see it, is about making the code as uncluttered/easy to read, semantically dense, and free of lies as we can. So I would ask: What information are we trying to transmit with the global includes: The only answer I can think of, is to have a list of all types referenced. I don't think this information has real value in this case. Additionally: If we do this, we would, to be complete, also need to include the classes in the same namespace, and I think we should definitely not do that. On the other hand we create a lot of noise and more work to keep things clean and we loose the backslash as a marker in the code that we are addressing a global type. This I would also not consider to be valuable information, though. So, from where I stand there is only one strong argument: We create clutter and work. This is why I would kindly ask you to remove them again and to not add them.
Please don't add @throws doc-blocks.
Could you clarify why @throws tags are discouraged? They help annotate methods that might throw exceptions, making it easier to identify where custom error handling is needed, especially within an IDE.
This is the same argument as before: I don't think this information helps us, as most of the errors will be thrown outside of our component and we mostly don't intend to react to them. Exceptions in ILIAS are mostly about things going sideways in an unrecoverable way and we want them to just bubble. In the few cases they are not, as far as I can see, we would need to deal with them as early as possible and we should not let them bubble. So there is no interesting information there as far as I can see. Additionally the exceptions are mostly thrown outside of our code and thus they could also change without us noticing and we would need to invest quite some work to avoid introducing lies into our code. And finally they make the code more cluttered and less readable. All of this for me points very strongly at not including the statements.
I hope this is understandable otherwise I'm at your disposal.
Best, @kergomard
hi @kergomard & @thojou - is this PR still a thing now or only after the beta phase of ILIAS 10?
Hi all
I think this can be merged whenever, but first, from where I stand, it needs a little more love ;-) .
Best, @kergomard
Hi all,
this PR is currently not compatible anymore. Due to other priorities we decided to close this for now. We do not want to throw away these changes completly, but they need major rework. A new PR will be made when everything gets sorted out.
Best regards @matheuszych