Split up TestUtils module

[penelopeysm]

This seems like a reasonable first step towards #550 and TuringLang/Turing.jl#2307.

This PR just breaks src/test_utils.jl up into thematically related bits, so as far as Julia is concerned this is a no-op. But, not all functionality in src/test_utils.jl is created equally, and this separation lets us discuss where the different things should go.

My initial thoughts:

• What's now src/test_utils/contexts.jl and src/test_utils/varinfo.jl are only really meant to be used inside DPPL tests, as far as I can tell. I reckon these should go into a DynamicPPLTestExt.
• src/test_utils/sampler.jl isn't used inside the DPPL tests, but is used in the Turing tests. I don't have a strong opinion on this but I think I would slightly prefer if they were moved to Turing as that is closer to the point of use.
• The demo models (and the interface defined for them) are definitely used outside of DPPL and so we should probably expose this functionality directly from DPPL. (This list could potentially be extended further for purposes of AD testing Refactoring AD Tests Turing.jl#2307). Thus, we should keep them inside src/. However, I think we shouldn't call the models TestUtils – I think we should give them their own module, like DemoModels.

I've avoided making any changes to the code itself because those changes would get lost in the diff.

[coveralls]

Pull Request Test Coverage Report for Build 11937950949

Details

• 371 of 429 (86.48%) changed or added relevant lines in 5 files are covered.
• 26 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-0.02%) to 84.334%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/test_utils/model_interface.jl	7	9	77.78%
src/test_utils/contexts.jl	18	24	75.0%
src/test_utils/sampler.jl	0	17	0.0%
src/test_utils/models.jl	323	356	90.73%

Files with Coverage Reduction	New Missed Lines	%
src/model.jl	1	93.68%
src/test_utils.jl	1	50.0%
src/varinfo.jl	6	86.3%
src/simple_varinfo.jl	6	86.6%
src/threadsafe.jl	12	57.76%

Totals
Change from base Build 11934706726:	-0.02%
Covered Lines:	3553
Relevant Lines:	4213

---

💛 - Coveralls

[codecov]

Codecov Report

Attention: Patch coverage is 86.48019% with 58 lines in your changes missing coverage. Please review.

Project coverage is 84.35%. Comparing base (ba490bf) to head (dded037).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/test_utils/models.jl	90.73%	33 Missing ⚠️
src/test_utils/sampler.jl	0.00%	17 Missing ⚠️
src/test_utils/contexts.jl	75.00%	6 Missing ⚠️
src/test_utils/model_interface.jl	77.77%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #719   +/-   ##
=======================================
  Coverage   84.35%   84.35%           
=======================================
  Files          30       35    +5     
  Lines        4211     4212    +1     
=======================================
+ Hits         3552     3553    +1     
  Misses        659      659           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mhauru]

Definitely an improvement. I obviously didn't actually check whether any code was changed, but assuming this is all just moving stuff, happy with it.

[willtebbutt]

I second @mhauru 's review -- this seems eminently sensible to me.

[torfjelde]

Good stuff @penelopeysm :)

I don't have a strong opinion on this but I think I would slightly prefer if they were moved to Turing as that is closer to the point of use.

This is sort of an issue of testing though IMO rather than that the functionality should live in Turing.jl.
As an example, DynamicPPL.Sampler isn't actually used in DynamicPPL.jl itself, but only sets it up so Turing.jl can implements its InferenceAlgorithm stuff.
Moreover, there are other AbstractMCMC.AbstractSamplers that would like to test against DynamicPPL.jl, and they shouldn't have to import Turing.jl to do so, e.g. MCMCTempering.jl currently does this.

[penelopeysm]

Ok, I'll keep samplers in TestUtilsExt then!

[torfjelde]

Wait, are we moving TestUtils into an extension?

[penelopeysm]

That's what I proposed! I was probably going to move it as a separate PR because that will be breaking.
Although it's a bit messy because of how extensions work – the functions will have to be defined inside src/test_utils.jl, whereas the methods are implemented inside DynamicPPLTestExt.

[torfjelde]

I was probably going to move it as a separate PR because that will be breaking.

Aye, that's what I understood originally.

Although it's a bit messy because of how extensions work – the functions will have to be defined inside src/test_utils.jl, whereas the methods are implemented inside DynamicPPLTestExt.

This is the part I find a bit confusing 😕 But yeah, not too opinionated about it.

[penelopeysm]

It is rather unfortunate that extensions work that way :/

I can make the PR and we can discuss the pros/cons there!

[penelopeysm]

A bit annoyed by how the merge queue makes us run CI twice 😅