Skip to content
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

Refactoring: Decouple API implementation from config structures #336

Draft
wants to merge 2 commits into
base: refactor-tests
Choose a base branch
from

Conversation

madmike200590
Copy link
Collaborator

@madmike200590 madmike200590 commented Feb 8, 2022

The aim of this PR is to get rid of all dependencies between SystemConfig and API implementations (like AlphaImpl, NaiveGrounder, DefaultSolver etc).
The idea is that these objects should get their dependencies at the time of construction from an external source (AlphaFactory as it is implemented now) and not need to be aware of any configuration mechanisms or structures themselves.

The same mechanism of obtaining Alpha instances (only through AlphaFactory) has been applied to all tests that test complete workflows (i.e. tests constituting end-to-end-tests in that they test the complete system rather than a single unit of code) as well. These tests (everything annotated RegressionTest or AggregateRegressionTest) have been moved to the alpha-solver module and adapted to be parameterized using a SystemConfig which is used to obtain an Alpha instance from the factory.

Additional changes:

  • renamed ASPCore2Program to InputProgram
  • removed config option for grounder name; it doesn't seem realistic that we get an alternative implementation in the short- or mid-term that would make such an option necessary
  • removed "deterministic" CLI option - instead select a seed directly
  • pulled code used in tests across multiple modules into separate source folder alpha-core/src/testFixtures that can be shared between modules
  • fix minor bug in StratifiedEvaluation: Avoid generating duplicate facts by using a Set rather than List as intermediate storage for newly derived rule heads

assumeTrue(solver instanceof StatisticsReportingSolver);
collectAnswerSetsAndCheckNoGoodCounterStatsByType(solver, 4, 0, 0, 0);
}
// TODO Why are these tests config-dependent if they use a grounder mock and seem rather like unit tests on solver statistics?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AntoniusW see TODO: I don't see a reason for these tests to be part of this class, i.e. I think these should be unit tests (and not parameterized with different configs). Let me know if you agree and I can move those to an appropriate class in the alpha-core module.

AtomStore atomStore = new AtomStoreImpl();
assertEquals(GrounderMockWithBasicProgram.EXPECTED, buildSolverForRegressionTest(atomStore, new GrounderMockWithBasicProgram(atomStore), cfg).collectSet());
}
// TODO @AntoniusW what are these? Can we get rid of them? If not, where do I move them?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AntoniusW see TODO: The commented-out test cases don't seem to be following the pattern of an end-to-end system test of all other tests in this class. Should we move these to some other location and remove the parameterization?

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #336 (1b04d74) into refactor-tests (515949e) will decrease coverage by 32.76%.
The diff coverage is 34.24%.

Impacted file tree graph

@@                  Coverage Diff                  @@
##             refactor-tests     #336       +/-   ##
=====================================================
- Coverage             71.11%   38.35%   -32.77%     
+ Complexity             2167     1200      -967     
=====================================================
  Files                   182      185        +3     
  Lines                  8033     8073       +40     
  Branches               1424     1422        -2     
=====================================================
- Hits                   5713     3096     -2617     
- Misses                 1938     4639     +2701     
+ Partials                382      338       -44     
Impacted Files Coverage Δ
.../tuwien/kr/alpha/app/config/CommandLineParser.java 82.47% <ø> (-0.42%) ⬇️
...tuwien/kr/alpha/core/grounder/GrounderFactory.java 0.00% <0.00%> (-50.00%) ⬇️
...c/tuwien/kr/alpha/core/grounder/NaiveGrounder.java 45.38% <0.00%> (-35.19%) ⬇️
...tuwien/kr/alpha/core/parser/ProgramParserImpl.java 65.30% <0.00%> (-12.25%) ⬇️
...wien/kr/alpha/core/programs/NormalProgramImpl.java 100.00% <ø> (ø)
.../at/ac/tuwien/kr/alpha/core/programs/Programs.java 0.00% <ø> (-50.00%) ⬇️
.../programs/transformation/EnumerationRewriting.java 0.00% <0.00%> (-85.72%) ⬇️
...transformation/NormalizeProgramTransformation.java 0.00% <0.00%> (-100.00%) ⬇️
...programs/transformation/PredicateInternalizer.java 0.00% <0.00%> (-74.20%) ⬇️
.../programs/transformation/StratifiedEvaluation.java 0.00% <0.00%> (-97.80%) ⬇️
... and 110 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 515949e...1b04d74. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant