-
Notifications
You must be signed in to change notification settings - Fork 438
Bg21 test case #1551
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
Bg21 test case #1551
Conversation
|
Thank you for your contribution to OED, before we continue please make sure all contributors have signed the CLA. Thank you. |
|
Hello, thank you for reminding us. We all signed the CLA. |
|
Hello, For clarification, do you mind confirming/revising our understanding of the test case? We believe the purpose of BG21 is to test that OED's APIs respond with a null response when we request 76 days of meter data to it while we only have a dataset of 75 days. |
|
You are almost correct but the basic idea is right. To quote Steve, the maintainer of the project, directly "It isn't really a null response but OED sends back no bar data to indicate none exists." |
|
All contributors have signed the CLA, the test case works as expected, and the changes look good. |
Let me clarify what I told @Chocopepero: OED tries to only include bars where readings are stored across the days in the desired bar. In this case the user asked to graph 76 days but there are only 75 days of readings so the bar would be missing a day. Thus, OED returns no data to indicate this. |
huss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to @JazzJE, @zihuan3 & @RussianGravy for their first contribution to OED. I've made a comment to address. Please let me know if you have any thoughts/questions.
| //get unit ID since the DB could use any value. | ||
| const unitId = await getUnitId('kW'); | ||
| // Load the expected response data from the corresponding csv file | ||
| const expected = await parseExpectedCsv('src/server/test/web/readingsData/expected_bar_group_ri_15-20_mu_kWh_gu_kWh_st_-inf_et_inf_bd_76.csv'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design document for testing specifies expected_bar_group_ri_15-20_mu_kW_gu_kW_st_-inf_et_inf_bd_76.csv for the test file. It is subtile but it is kw not kWh (twice). I don't think this will change the result but it should be used. This also means including this file in the PR as described in the general test information.
|
Hopefully that's suitable enough. Please let us let know if there are any other changes needed to be made! |
|
Apologies for the delay. The requested change, both the expected file name and the new included file, do match exactly to what's on the testing document. Apologies again for missing that. |
huss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updated code. I concur with @Chocopepero that everything is good. Congratulations to @JazzJE, @zihuan3 & @RussianGravy on your first accepted contribution to OED.
Description
Added test case BG21 to readingsBarGroupFlow.js. Ran tests with 'npm run test'; all the tests passed.
Contributors:
Jedrick Espiritu @JazzJE [email protected]
Deandre Huang @zihuan3 [email protected]
Valentine Jones-Metsiou @RussianGravy [email protected]
Partially Addresses #962
Type of change
Checklist
Limitations