alpha-asp / Alpha

A lazy-grounding Answer-Set Programming system
BSD 2-Clause "Simplified" License
58 stars 10 forks source link

Refactor tests on modularized code structure #316

Closed madmike200590 closed 2 years ago

madmike200590 commented 2 years ago

(clone of PR #308)

This PR is a follow-up to PR #274. Following the changes to the overall code structure and API introduced through modularization, I took the opportunity to get started on some refactorings of Alphas unit tests. Specifically, this PR aims to

Reduce dependencies and narrow down tested code for unit tests

In order to make our unit tests more precise and avoid localized errors failing lots of tests, I removed unnecessary dependencies from most tests. Especially, we don't want to have tests of core components like gronder and solver depend on the complete program preprocessing workflow including parser and rewritings.

Move functional tests to alpha-solver module

Most tests that test rewriting and preprocessing features, as well as all tests that just pass programs into an Alpha instance and verify returned answer sets should be considered functional tests rather than unit tests since they test complete features through the complete workflow. These were moved into the alpha-solver module since the default implementation of Alpha resides there.

Mark functional and integration tests for further attention

In order to improve build performance and better distinguish between unit-, integration-, functional- and performance-tests, these should be separated into separate tasks in the build (of which some should probably only run in CI) process. This has not yet been realized yet, but tests that should probably be treated as integration tests or functional tests were marked with appropriate TODOs in preparation for that refactoring step. (Issue TBD)

AntoniusW commented 2 years ago

@madmike200590 Since you closed the other PR due to a "broken history", I have to ask whether the history we have here is the intended one? It seems to contain all commits from the modularization (again). Is rebasing a good (and easily executable) idea for this branch?

madmike200590 commented 2 years ago

@madmike200590 Since you closed the other PR due to a "broken history", I have to ask whether the history we have here is the intended one? It seems to contain all commits from the modularization (again). Is rebasing a good (and easily executable) idea for this branch?

Yes, the history here is the intended one. I closed the other PR because I had some issues getting git to start a proper merge locally - in hindsight, that was mostly my own mistake, but I wanted to make sure that the "shifted" (since modules was merged while #308 was open) base of the PR was not to blame.

As for rebasing, yes, that might be elegant - I tried and reverted it since it's unfortunately not easily executable.

The reason for my merging troubles was that we have one squashed commit for PR #274 in master, while we have the individual commits for the same changes plus commits further modifying the same files here. Since the history git would need to know that the changes from here have to be applied on top of what's in master is not available (because it got squashed), I had to do a huge manual merge.

As for merging this PR, that means I'd like to do a simple merge and keep the history as it is here - otherwise we'll have the same problem with every branch that got branched from modules before merging PR #274. I'll investigate if we can still get rid of "WIP" commits by selectively squashing smaller sets of commits.

codecov[bot] commented 2 years ago

Codecov Report

Merging #316 (3cea9cc) into master (fd5e5ba) will increase coverage by 0.87%. The diff coverage is 64.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #316      +/-   ##
============================================
+ Coverage     69.51%   70.38%   +0.87%     
- Complexity     2092     2135      +43     
============================================
  Files           182      182              
  Lines          8006     8016      +10     
  Branches       1416     1417       +1     
============================================
+ Hits           5565     5642      +77     
+ Misses         2086     1996      -90     
- Partials        355      378      +23     
Impacted Files Coverage Δ
...kr/alpha/commons/externals/AspStandardLibrary.java 78.26% <ø> (ø)
...mmons/externals/BinaryPredicateInterpretation.java 0.00% <ø> (ø)
...xternals/BindingMethodPredicateInterpretation.java 11.11% <ø> (ø)
...c/tuwien/kr/alpha/commons/externals/Externals.java 8.57% <ø> (ø)
.../commons/externals/IntPredicateInterpretation.java 50.00% <ø> (ø)
...commons/externals/LongPredicateInterpretation.java 0.00% <ø> (ø)
...mmons/externals/MethodPredicateInterpretation.java 15.78% <ø> (ø)
...s/externals/NonBindingPredicateInterpretation.java 33.33% <ø> (ø)
...ons/externals/SuppliedPredicateInterpretation.java 0.00% <ø> (ø)
...ommons/externals/UnaryPredicateInterpretation.java 0.00% <ø> (ø)
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fd5e5ba...3cea9cc. Read the comment docs.

madmike200590 commented 2 years ago

Closing this PR due to issues with git history (individual commits of #274 got squashed upon merge), replaced by #335