search

assaf / vanity

Experiment Driven Development for Ruby
http://vanity.labnotes.org
MIT License
1.55k stars 269 forks source link

Use shared tests for all adapters #316

Closed urbanautomaton closed 7 years ago

urbanautomaton commented 7 years ago

Hi there,

As mentioned in #309, I've been able to apply the tests introduced for the mock adapter to the other adapters, with only a few tweaks here and there.

I hope that the individual commit messages are explanatory, but some highlights of the changes made are:

The last one was the main stumbling block - it seems like the ability to track multi-valued metrics was introduced a while back, but returning them isn't supported by the main RedisAdapter, and I can't find any evidence that they're actually used (at least in vanity itself - third party code might, I guess). In the short term I've just loosened the test to only test single-value tracking, but this might need a second look later.

Any feedback on this is very welcome. If the diff is too large to sensibly review I could always split this out into separate PRs, one for each adapter, so just let me know.

It probably won't pass on travis just yet because of the way the test setup for the different storage options is performed (I'm trying to work out how best to handle this), but the tests do all pass locally.

Cheers, Simon

phillbaker commented 7 years ago

@urbanautomaton awesome! This is something that I had on my list as well, but never got around to it.

For the testing, I was thinking that perhaps it might be easier to not parameterize the build with the datastore, but to always run all the datastores? So the test matrix would switch from ruby version/db/rails version to just ruby version/rails version. That'd speed up travis builds a bit and remove some of the special casing in the test suite. I'll pull down your branch and see if I can get that working. What do you think?

urbanautomaton commented 7 years ago

For the testing, I was thinking that perhaps it might be easier to not parameterize the build with the datastore, but to always run all the datastores?

If that's possible, I think it might be preferable. I gave this approach a quick look, but was worried that there's a fair bit of the test suite that's kinda implicitly an integration test against the currently-selected data store. I felt like we'd end up needing to repeat a lot of stuff for each store, which wouldn't be substantially different than the existing parameterisation. If it's possible though then definitely the extra benefit of cutting down the build matrix would be worth it IMO.

In the meantime I've pushed a quick bodge (only run the relevant adapter tests in their own parameterised job) which makes the build green.

phillbaker commented 7 years ago

but was worried that there's a fair bit of the test suite that's kinda implicitly an integration test against the currently-selected data store.

Yup, definitely the case. Room for improvement 😀 !

In the meantime I've pushed a quick bodge (only run the relevant adapter tests in their own parameterised job) which makes the build green.

Sounds good!

Had a quick clarifying question, but other than that, I'm good to merge. Thanks for all the hard work!