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

Mock backend #1116

Merged
merged 11 commits into from
May 19, 2021
Merged

Mock backend #1116

merged 11 commits into from
May 19, 2021

Conversation

martinmodrak
Copy link
Contributor

This is not intended to directly merge, just to show a proof-of-concept implementation of a "mock" backend (#1115) that allows cheap integration tests of brm and brm_multiple calls. If you find the general idea/direction OK, I would have a go at making the implementation a bit cleaner, most notably to make the "mock" backend accessible only from testing code and not for regular users of the package.

The main idea is that with backend = "mock" you can pass additional arguments that tell the backend exactly what to return instead of the stanfit and can also force an error in any step of the processing (probably not very useful, but still could check if error processing is OK and errors are not consumed somewhere). By default the "mock" backend also checks that the model can be parsed to C++ but does not compile it.

New tests were added in tests.brmR to show how this could be used.

Happy to hear any feedback.

@martinmodrak
Copy link
Contributor Author

So I noticed that the official testthat guide actually recommends having test fixtures (such as our mock backend) a part of the main codebase:

To solve this problem we create a test fixture, which we place in R/test-helpers.R so that’s it’s available for both testing and interactive experimentation:

(https://testthat.r-lib.org/articles/test-fixtures.html#case-study-usethis)

I am not sure I like the idea (I am used to having testing code strictly separate from main code), but maybe there is something to it? There is definitely a benefit in that packages depending on brms could use the mock backend to write their tests, which could make this worth exposing.

So if you are OK exposing the mock backend more broadly than I think the code is actually ready to review for merging.

@martinmodrak martinmodrak changed the title Mock backend - discussion and proof-of-concept Mock backend Mar 11, 2021
@paul-buerkner
Copy link
Owner

Great, thank you! I will take a closer look soon. What do you thhink of giving the backend a more visually pleasing name, for example "test" instead of "mock"?

@martinmodrak
Copy link
Contributor Author

The name doesn't matter much. I just think "mock" is well understood and commonly used in testing contexts (https://en.wikipedia.org/wiki/Mock_object), but apparanetly people also use "stub" for a simple implementation as the one we have here (https://stackoverflow.com/questions/2665812/what-is-mocking). Could still be "test" it really is just an aesthetic choice.

@paul-buerkner
Copy link
Owner

Thanks! I learn new things every time :-)

@paul-buerkner paul-buerkner added this to the brms 2.15.0++ milestone May 17, 2021
@paul-buerkner
Copy link
Owner

I have finally managed to take a look at the PR and it already looks quite good to me. Sorry that it took me so long.

I am only a bit hesitant to call the passed argument stanfit given that it can easily be mistaken for the fit argument. Shall we rename it to something that makes more explicit we are using a "mock" fit object?

@jgabry
Copy link
Contributor

jgabry commented May 18, 2021

Cool idea!

To solve this problem we create a test fixture, which we place in R/test-helpers.R so that’s it’s available for both testing and interactive experimentation:

(https://testthat.r-lib.org/articles/test-fixtures.html#case-study-usethis)

I am not sure I like the idea (I am used to having testing code strictly separate from main code), but maybe there is something to it? There is definitely a benefit in that packages depending on brms could use the mock backend to write their tests, which could make this worth exposing.

Even if the code is put in the R directory instead of the tests directory it won't be usable by other developers in their own tests unless the relevant functions are also exported in the NAMESPACE (otherwise they'd need to use ::: and CRAN would complain). That said, maybe the functions you're adding don't need to be exported, other developers can just directly use backend="mock" and those functions are used behind the scenes?

@martinmodrak
Copy link
Contributor Author

I am only a bit hesitant to call the passed argument stanfit given that it can easily be mistaken for the fit argument. Shall we rename it to something that makes more explicit we are using a "mock" fit object?

That's very sensible. I changed the argument to mock_fit which should avoid any confusion...

That said, maybe the functions you're adding don't need to be exported, other developers can just directly use backend="mock" and those functions are used behind the scenes?

That's exactly the idea.

@paul-buerkner
Copy link
Owner

I will probably make some minor aesthetic edits before I merge. Is there anything you want to add before that?

@martinmodrak
Copy link
Contributor Author

martinmodrak commented May 18, 2021 via email

@paul-buerkner
Copy link
Owner

Thank you for working on this issue! Merging now.

@paul-buerkner paul-buerkner merged commit b59c4b1 into paul-buerkner:master May 19, 2021
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.

None yet

3 participants