#3990 Add csv export tests for read_events feature toggle enabled #4260
Conversation
|
@danielabar (nods) We have recently started a concentrated effort on improving our flakey tests. Calling the above list out to @elasticspoon, since he's been doing a lot of work in that area. |
|
|
||
| it "generates header with Storage Location fields followed by alphabetized item names" do | ||
| get storage_locations_path(default_params.merge(format: response_format)) | ||
| expect(response.body.split("\n")[0]).to eq([StorageLocation.csv_export_headers, item1.name, item2.name, item3.name, item4.name].join(',')) |
There was a problem hiding this comment.
There's no reason to reference item1.nameetc. when we actually know what the name is (A, B etc.).
| 5 | ||
| ].join(",")) | ||
| end | ||
| end |
There was a problem hiding this comment.
Usually for CSVs, I check the results something like this:
csv_data = <<~CSV
header1,header2,header3
r1cell1,r1cell2,r1cell3
#{dynamic_value1},r2cell2,r2cell3
CSV
expect(results).to eq(csv_data)There was a problem hiding this comment.
I can update the tests to use this technique (actually with this, can probably get all of it into just one test).
Only tricky thing could be one of the csv rows is for a storage location that comes from either seeds or fixtures, not data explicitly generated by the test. For example, the Smithsonian Conservation Center is not from the test:
Name,Address,Square Footage,Warehouse Type,Total Inventory,A,B,C,D
Smithsonian Conservation Center,"1500 Remount Road, Front Royal, VA 22630",100,Residential space used,0,0,0,0,0
Storage Location with Duplicate Items,"1500 Remount Road, Front Royal, VA 22630",100,Residential space used,1,0,0,1,0
Storage Location with Items,"1500 Remount Road, Front Royal, VA 22630",100,Residential space used,3,1,1,1,0
Storage Location with Unique Items,"1500 Remount Road, Front Royal, VA 22630",100,Residential space used,5,0,0,0,5
I can add an assertion for it, but if the seeds/fixtures should change in the future, this test would break.
Thoughts on how to deal with this?
There was a problem hiding this comment.
Another observation: That storage location that's not generated explicitly by the test uses the StorageLocation::WAREHOUSE_TYPES.sample for warehouse_type, resulting in a different csv output each time which would make this test flaky.
I actually ran into that earlier with my own factory created storage locations, which I fixed by always selecting a specific warehouse type when creating, eg:
let(:storage_location_with_items) { create(:storage_location, name: "Storage Location with Items", warehouse_type: StorageLocation::WAREHOUSE_TYPES.first) }Potential Solutions:
- Figure out where that additional storage location is coming from and try to remove it from fixtures or seeds (but this might break other tests).
- Process the
response.bodyto remove that row before using it in test expectation. But it might not always be guaranteed to be the first data row, so this could result in flakiness. - Use my current technique of finding each specific csv row to verify by storage location name, and assert on those independently. (but this either results in multiple tests, or I could collapse it into a single test but then it would have multiple assertions - is that ok?)
There was a problem hiding this comment.
Sorry, just realized that extra storage location actually is created by the test, just not the new test(s) I added. It's created in thebefore block for describe "GET #index". In this case, I think it could be modified to always have the same warehouse type, and then the csv test can have consistent data to verify.
There was a problem hiding this comment.
I've made the update to use a heredoc to verify generated csv contents in a single test.
1. Use actual values of item names when verifying csv header. 2. Use heredoc to verify contents of generated csv in a single test.
|
@danielabar: Your PR |
Resolves #3990
Description
Based on issue discussion, it was determined that when the
read_eventsfeature toggle is enabled, the storage location csv export is correct. In that a storage location with an item that's not present in any of the other storage locations will have the inventory for that item located in the correct column for that item. And for cells with none of the items, a0is displayed.So it's only when
read_eventsfeature toggle is disabled, then the code uses thegenerate_csvfrom the Exportable concern, with incorrect input which leads to the bug.It was decided rather than spending time to fix the path the code takes when
read_eventsis off, to just add tests to cover the csv exporting whenread_eventsis on. This PR adds tests for both header, and data rows generation.For reference these comments in the issue have further analysis details:
Type of change
How Has This Been Tested?
bin/start, navigate tohttp://localhost:3000/flipperand sign in with the admin account, then add toggleread_eventsand enable for all.http://localhost:3000/diaper_bank/storage_locations, and add a new storage location0in this column.Run the tests that verify this behaviour:
bin/rspec spec/requests/storage_locations_requests_spec.rb:69Note when I run the full test suite, get 3 failing system tests, but this branch doesn't have any code changes: