DEV-1520 - get cost report working #335
Conversation
| @@ -131,25 +138,6 @@ | |||
| end | |||
| end | |||
|
|
|||
| describe "#matching_clusters" do | |||
There was a problem hiding this comment.
We aren't implementing this at all now, so tests are gone.
spec/spec_helper.rb
Outdated
| @@ -83,6 +80,11 @@ | |||
| end | |||
|
|
|||
| config.before(:each) do | |||
| # mock HT member data to use in tests. Tests may change this data, so | |||
There was a problem hiding this comment.
I don't know why this worked before. What I observed was that data added with add_temp leaked between tests and caused issues. This seemed like a reasonable fix.
| @@ -91,28 +85,43 @@ def add_cluster_htitem(*htitems) | |||
|
|
|||
| describe "#cost_per_volume" do | |||
| it "calculates cost per volume" do | |||
| expect(cr.cost_per_volume).to eq(cr.target_cost / cr.num_volumes) | |||
| # 3 volumes, target cost $10 | |||
There was a problem hiding this comment.
I found it hard to understand what the state of the database was given the various before blocks and additional stuff being added. I figured being explicit in each test about what's there was the best option.
| expect(cr.cost_per_volume).to eq(cr.target_cost / cr.num_volumes) | ||
| # 3 volumes, target cost $10 | ||
| add_cluster_htitem(spm, mpm, ht_allow) | ||
| expect(cr.cost_per_volume).to be_within(0.01).of(3.33) |
There was a problem hiding this comment.
I also wanted to be explicit here about the expected result, rather than recomputing it using the code from the class. That makes the tests somewhat more brittle, but that brittleness is kind of endemic in this class anyway given the fixture data..
There was a problem hiding this comment.
The cost report tests might be (or at least have been) some of the most brittle in this repo. Some of that we just have to live with, but I appreciate the things you are doing to at least make things more explicit.
aelkiss
force-pushed
the
DEV-1520-cost-report
branch
from
21e576f to
cfa7b85
Compare
|
Rebased; ready for review. See comments inline. |
| expect(cr.cost_per_volume).to eq(cr.target_cost / cr.num_volumes) | ||
| # 3 volumes, target cost $10 | ||
| add_cluster_htitem(spm, mpm, ht_allow) | ||
| expect(cr.cost_per_volume).to be_within(0.01).of(3.33) |
There was a problem hiding this comment.
The cost report tests might be (or at least have been) some of the most brittle in this repo. Some of that we just have to live with, but I appreciate the things you are doing to at least make things more explicit.
|
@mwarin thanks for the feedback. I will try to address those items. |
* remove cluster_tap_save; new add_cluster_htitem helper for putting it in the hathifiles table and adding its OCNs to a cluster * replace mongo queries from cost report (DEV-1523) * remove matching_clusters (DEV-1522) * alo -> allow; dni -> deny * many tests are failing with wrong numbers, will investigate.
* add additional collections for use in integration tests * add note about reliance on specific organizations listed here * reset organizations/collections between tests
* billing_entity -> collection code for htitems & associated reorganization * clarify for many tests what exactly we expect to be present in the database; avoid before block that creates stuff leaking scope * use cr.compile_frequency_table instead of repeating that code in tests (which relied on matching_clusters, which is no longer relevant for mariadb since we iterate directly over htitems)
This is a bit messy in terms of spelled-out fixture data being included in the tests since we don't have the ability to directly load JSON data any more.
aelkiss
force-pushed
the
DEV-1520-cost-report
branch
from
cfa7b85 to
974e491
Compare
* remove trivial methods in costreport for counts * Move data for integration spec to support context * cluster_tap_save -> load_test_data
aelkiss
force-pushed
the
DEV-1520-cost-report
branch
from
ded40e4 to
6a8c867
Compare
|
@mwarin I've addressed feedback and will go ahead and merge once specs & standardrb are happy. |
This incorporates work from #329 and #333, and we will need to rebase after that's merged (and likely there will be more changes needed after #331 is merged as well). The relevant work here is my commits at the end. I'll leave some comments inline about specific things I'd like to think about.