search

metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

ci: test against minimum required bbi version #545

Closed kyleam closed 2 years ago

kyleam commented 2 years ago

The first commit of this PR update the mpn:oldest build to use bbi version 3.0.2, the value of bbr.bbi_min_version. The goal is to keep bbr compatible (and the tests passing) with the minimum version that's specified there.

The other two commits adds some skipping/conditional logic to the tests to avoid some failures when running with the minimum required bbi.

I talked with @seth127, and he is on board with the general idea, but of course please chime in with any comments on the specifics.

barrettk commented 2 years ago

@kyleam do we want a requirement/story for this, or do you think its unnecessary? Not a whole lot changed, but I feel like the compatibility with older versions of bbi might be something to keep track of (perhaps the release notes are sufficient though)

seth127 commented 2 years ago

do we want a requirement/story for this

No, this is more just "good housekeeping"

kyleam commented 2 years ago

@kyleam do we want a requirement/story for this, or do you think its unnecessary?

Hmm, I was considering all of these changes developer-facing meta stuff related to the CI that don't need to have requirements/stories. But...

Not a whole lot changed, but I feel like the compatibility with older versions of bbi might be something to keep track of (perhaps the release notes are sufficient though)

... I see the point that compatibility with older versions fits a user story

But, in terms of this PR, I think my view is that bbr.bbi_min_version isn't changing, and we're just cleaning up some tests that were a bit too forward-thinking in their expectations. So, while we may want a story at some point, this PR doesn't need to be responsible for taking that on.

Does that make sense? (Of course, if someone has a story right at the tip of their tongue and has a strong opinion that we need it, it's very little work to include that here and would be fine by me. edit: on the test end, I guess we'd need a simple test that checks the value of the option so that there was something to link?)

(though I'm off the top of my head, I'm not sure what exact wording we'd want)

Us adding a build to the CI that tests with the minimum version and cleaning up some tests doesn't feel NEWS-worthy to me. However, if we were changing bbr.bbi_min_version, that's definitely something that should be highlighted.

seth127 commented 2 years ago

This looks good to me. Thanks y'all.