Skip to content

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

2 participants