Replace duplicated raw export size calculations with a shared estimator - #1686
Replace duplicated raw export size calculations with a shared estimator#1686stockingstocker wants to merge 8 commits into
Conversation
…esent in shared file
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@stockingstocker Thank you for another contribution. Review and testing found it works well. I have made a few comments to consider. Please let me know if anything is not clear or you have thoughts/questions.
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| const chai = require('chai'); |
There was a problem hiding this comment.
While this works, many files get the chai, mocha & expect from common file to keep them consistent across files. I think that would be better. Having said that, OED probably should open an issue to do that across all tests to be consistent, assuming it causes no problem. Would you like to do that or should I? This should also get rid of the blank line between items as OED prefers.
There was a problem hiding this comment.
Thanks for catching that. I was working from habit and forgot about OED’s common test file.. For the issue, I would be happy to open it after I take a closer look at the existing test files.
|
|
||
| mocha.describe('Raw Export File Size Estimator', () => { | ||
| mocha.it('returns zero for zero readings', () => { | ||
| expect(estimateRawExportSizeMB(0)).to.be.closeTo(0, 0.001); |
There was a problem hiding this comment.
Is there a specific reason you use 0.001 tolerance on these tests? In readings (see src/server/util/readingsUtils.js) it has a DELTA that is stricter. Stricter is better when it works. I would have thought this would be basic quasi-random numerical variation in the final digit given what is being tested. So, I tried 1e-15 and that worked fine. Maybe that should be used and set to a const in the file..
There was a problem hiding this comment.
I didn't notice the delta value in readingsUtils.js. I now see that I gave the tests too much wiggle room. I will make the change
|
|
||
| const ESTIMATED_KB_PER_RAW_READING = 0.082; | ||
| const KB_PER_MB = 1000; | ||
| /** |
There was a problem hiding this comment.
Could you please add a blank line before the JSDco.
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
|
|
||
| const ESTIMATED_KB_PER_RAW_READING = 0.082; |
There was a problem hiding this comment.
I have two thoughts. First, given the estimate is in MB, would it be easier to make this const be in MB and avoid the KB_PER_MB conversion? I might make it 8.2e-5 so it is easier to understand. Second, I'm wondering why this is exported. I could not find uses outside this file except in the d.ts file but then that usage does not seem to be used anywhere else. I'm uncertain this const would ever be used anywhere else in OED and encapsulating it to the function may make sense. Thus, I was wondering about removing the export, placing the const inside the function and removing it from the t.ds file. It worked fine for me.
There was a problem hiding this comment.
For the first point, I was keeping the structure of the formula the same as it originally was in the client but you're right that changing the estimate to be in MB makes a lot more sense now that its in a function. For the second point I was being safe just in case the values may be needed elsewhere but I realize now that encapsulating it for just the function is more reasonable.
|
I saw the update. I'm sorry that it is going to take a little while for me to get to this. Let me know if the delay becomes an issue for you. |
2 participants
Description
This PR moves the raw export file size estimation logic into a shared module used by both the client and server. The client uses the estimate to warn users and determine whether an export should proceed, while the server uses the same estimate to enforce the configured file size limit.
Previously,
exportThunk.tsandreadings.jseach contained their own copy of the file size estimation formula:count * 0.082 / 1000Keeping separate copies creates a risk that the client and server could use different values or formulas if one is updated without the other.
Now, the client and server import
estimateRawExportSizeMB()instead of calculating the estimate independently. Unit tests have also been added undersrc/server/test/utilto verify the estimator returns the expected values for:Fixes #1684
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
No limitations.