Add tests

[jamesmbaazam]

This PR adds tests for all the functions in the package to close #45.

[codecov-commenter]

Codecov Report

Merging #71 (8e78d6f) into main (88df856) will increase coverage by 36.32%.
The diff coverage is n/a.

❗ Current head 8e78d6f differs from pull request most recent head 537ceab. Consider uploading reports for the commit 537ceab to get more accurate results

@@             Coverage Diff             @@
##             main      #71       +/-   ##
===========================================
+ Coverage   62.50%   98.82%   +36.32%     
===========================================
  Files           8        8               
  Lines         424      424               
===========================================
+ Hits          265      419      +154     
+ Misses        159        5      -154     

see 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jamesmbaazam]

@pratikunterwegs @joshwlambert @Bisaloo any idea how to fix this failing test?

[pratikunterwegs]

@pratikunterwegs @joshwlambert @Bisaloo any idea how to fix this failing test?

Perhaps a stupid question, but have you changed the behaviour of the function in a way that would legitimately change the snapshot? In which case accepting the new snapshot would be the way out, I'd guess.

[jamesmbaazam]

@pratikunterwegs I haven't. Normally, that's the output you'd see in Rstudio when you first run the test and it'd go away after you've accepted it. However, I don't know how to achieve that in the CI. I'm tempted to delete these tests at the cost of some coverage.

[joshwlambert]

@jamesmbaazam can you reproduce this issue locally? I've run the tests and calculated code coverage on this branch locally and everything works.

Have you tried re-running the failed workflow to check if it was a one-time issue?

[jamesmbaazam]

@joshwlambert It passes locally for me too but fails on the CI. I get the same error when I re-run the failed jobs.

[pratikunterwegs]

@jamesmbaazam from what I understand the snapshot fails as its components are being wrapped in covr:::count() calls. This seems to be because {covr} identifies the outputs as R expressions whose coverage should be checked. This just appears to be how {covr} works. If I'm right, this is part of trace_calls(). From the {covr} vignette:

The core function in covr is trace_calls(). This function was adapted from ideas in Advanced R - Walking the Abstract Syntax Tree with recursive functions. This recursive function modifies each of the leaves (atomic or name objects) of an R expression by applying a given function to them. If the expression is not a leaf the walker function calls itself recursively on elements of the expression instead.

This issue on using {covr} with {drake} appears to suggest that there is not much to be done about this when there's a conflict with static code analysis tools - you would probably be better off removing this snapshot test.

I've tried replacing the expect_snapshot(body(...)) command with expect_snapshot(writeLines(deparse(body(...)))), but since the wrapping happens when the function factory output is generated, it's included in the string, and the snapshot test fails anyway.

One alternative which I haven't tried and which just might work is converting the function factory output to a string internally, and returning a string instead of a closure. I think this would also be caught by {covr} and would probably result in the same issue.

Another issue which you might find insightful regarding code coverage for function factories: r-lib/covr#363

An alternative would be to create snapshot tests for the outputs of the generated functions, rather than the function body itself, and/or testing for the statistical correctness.

[jamesmbaazam]

@pratikunterwegs Thanks for looking into this. Really detailed explanation. I thought of it over the weekend and decided to remove the snapshot test for now as it doesn't make much sense. There's another test to ensure that the right distribution is passed to the spec argument. The discussion in the linked issue is succinct and insightful. In essence, there are just some things you can't test exactly.

[Bisaloo]

For an immediate fix of the issue at hand, you can add skip_on_covr() to these tests.

More generally, and slightly beyond the scope of this specific PR, I don't see much benefit for this function in its current state. As far as I can tell, It is used just once, in simulate_tree_from_pop() but it doesn't really simplify the code there because there is still branching based on offspring_dist value. While I appreciate the value of splitting the code in conceptually distinct units in the form of functions, even when they are called just one, there is a risk of ending up with code so nested that you have to jump in a giant rabbit hole each time you want to understand anything about the codebase. I think we are dangerously tilting in the rabbit hole direction here.

I see two potential solutions:

• removing get_offspring_func() and make the code part of the main simulate_tree_from_pop(). This is probably my preferred option in this specific case
• making sure that get_offspring_func() allows you to simplify the code in the parent simulate_tree_from_pop() function. This could be achieved by moving the argument checking code in get_offspring_func(), which allows you to remove the branching in get_offspring_func()

[jamesmbaazam]

Thanks @Bisaloo. I will implement the first option. Creating an issue to fix this.

[jamesmbaazam]

@pratikunterwegs Would you like to review this PR? I am hoping to merge it in by the close of Wednesday.

[pratikunterwegs]

Sure, will have feedback by tomorrow afternoon.

[pratikunterwegs]

I would suggest reorganising this file so that the mapping between the <epichains> and aggregated data created at the top, and the actual tests, are clearer for maintainers who are new to the package. You could create each object just before you test it, for instance.

I see that the class of the objects is tested in tests-simulate.R whereas this file tests methods, maybe a small comment saying that would be good here for future maintainers. Alternatively, you could combine the two files.

[jamesmbaazam]

You could create each object just before you test it, for instance.

Thanks. I'll make the change. This was how I organised it originally but noticed I was doing the same thing many times, so decided to move it to the top. But it does make sense to have it within the context in which they're tested for ease of reading. it seems to be a tradeoff between readability and efficiency. I've made the suggested changes here fdf45f8 and 395641d.

Alternatively, you could combine the two files.

I think I'll keep them separate to keep the script-to-test mapping organisation.

[pratikunterwegs]

I would suggest adding a check for the return type, which is currently a data.frame. Might be worth adding this to the method documentation as well, as users might be expecting an <epichains> object.

[jamesmbaazam]

I've added the tests here 01c5ef9. I'll create an issue for the documentation issue.

[pratikunterwegs]

Here it might be worth also checking that the data aggregated by "both" inherits from a list, whereas the other two inherit from data.frame. I think there should be a wider rethinking of classes in {epichains} as well to avoid this ambiguous inheritance structure

[jamesmbaazam]

Thanks for the suggestion for rethink the class structures. I'll raise an issue for further discussion. For now, I've added the suggested test here fdf45f8.

[pratikunterwegs]

I'm reminded by the recent {cfr} review that test descriptions that include function names can be brittle to function name changes - might be good to change that.

[jamesmbaazam]

I agree that they are brittle but I wonder if they are really that hard to maintain in a small code base like this. Even for "larger" code bases like that of dplyr, their tests like this one for across use function names.

Generic descriptions in long test files are hard to debug in my opinion.

[pratikunterwegs]

That was pretty much my logic in {cfr} too - but you suggested to make them more descriptive. I'm happy for the function names to stay in.

[jamesmbaazam]

Yeah, the suggestion was to more specific with the testing contexts. Maybe there was a lapse in communication.

[pratikunterwegs]

Thanks @jamesmbaazam - looks alright to me overall. I haven't really looked into {epichains} before. From what I can see, I think the tests cover the package functionality, and there seem to be tests for correctness so hopefully the chain simulation functions work as expected.

This PR is mostly adding tests, but one issue that I noticed is that <epichain> and <epichains_aggregated_df> objects can inherit from different base classes (data.frame and vector, and data.frame and list, respectively). I'm not sure whether it's a good idea to get into conditional inheritance in this way. This would probably make it difficult for future developers to easily account for what sort of object a function will return. If the data in each case (e.g. epichain_tree vs epichain_summary) is sufficiently different, perhaps it would be better to have separate classes. If you would find an overarching signature more convenient for some methods, the existing classes could be defined as abstract super-classes with sub-classes instead. Happy to discuss this further.

A minor point is that looking at the functions that are related to checking the offspring distribution function, you would be restricting users to pass functions that are available in {stats} and can thus be found by exists(). Are there likely to be cases where users would want to specify a function from another package, and pass it explicitly namespaced as "pkg::function"? If so, do you intend to support that, and would the check_*() functions need to account for it?

[jamesmbaazam]

This PR is mostly adding tests, but one issue that I noticed is that and <epichains_aggregated_df> objects can inherit from different base classes (data.frame and vector, and data.frame and list, respectively). I'm not sure whether it's a good idea to get into conditional inheritance in this way. This would probably make it difficult for future developers to easily account for what sort of object a function will return. If the data in each case (e.g. epichain_tree vs epichain_summary) is sufficiently different, perhaps it would be better to have separate classes. If you would find an overarching signature more convenient for some methods, the existing classes could be defined as abstract super-classes with sub-classes instead. Happy to discuss this further.

I've logged this here #79.

A minor point is that looking at the functions that are related to checking the offspring distribution function, you would be restricting users to pass functions that are available in {stats} and can thus be found by exists(). Are there likely to be cases where users would want to specify a function from another package, and pass it explicitly namespaced as "pkg::function"? If so, do you intend to support that, and would the check_*() functions need to account for it?

This issue of function look-up has been discussed extensively in #25 and #33 (comment).

[pratikunterwegs]

Thanks for the changes @jamesmbaazam - looks alright to me.

[jamesmbaazam]

Thanks for your thorough review as always.