Download full history - see issue 1468 #1504
Conversation
|
I read the contributing guidelines but those seems to be related only to the release procedure? |
Codecov Report
@@ Coverage Diff @@
## master #1504 +/- ##
==========================================
+ Coverage 81.40% 81.67% +0.27%
==========================================
Files 27 28 +1
Lines 2398 2467 +69
Branches 370 382 +12
==========================================
+ Hits 1952 2015 +63
- Misses 355 359 +4
- Partials 91 93 +2
Continue to review full report at Codecov.
|
|
Hoping for some feedback on this:) |
| self.assertIn("Custom string arg", stdout) | ||
|
|
||
| def test_csv_full_history_requires_csv(self): |
There was a problem hiding this comment.
This is one of the few times you'll hear me saying it, but I think this is a test we dont actually need :)
There was a problem hiding this comment.
I can remove it, but I don't see why you would not like to test handling of invalid parameter combinations? You are never going to get full coverage either if you do not test this.
There was a problem hiding this comment.
Strict coverage metrics dont really say a lot. But leave it in if you like, I guess it doesnt hurt
There was a problem hiding this comment.
It does not say anything about the functionality (requirements correctness), but I disagree that it does not say as lot. It says a lot about the general code quality. If you have tested the functionality, then what does all the uncovered code do?
But let's close that discussion as it is a little off topic :)
| def test_request_stats_full_history_csv(self): | ||
| self.stats.log_request("GET", "/test2", 120, 5612) | ||
| response = requests.get("http://127.0.0.1:%i/stats/requests_full_history/csv" % self.web_port) | ||
| self.assertEqual(200, response.status_code) |
There was a problem hiding this comment.
Maybe validate the actual content of the response as well?
There was a problem hiding this comment.
Sure, I can add that - it will be somewhat duplicating test_csv_stats_writer_full_history - but some duplication in tests are not a bad thing.
I can test for the header or for header and data - if testing for data I need to wait until data is present.
There was a problem hiding this comment.
if it takes a long time to test data and you do it somewhere else then maybe it is not necessary (but it would be nice)
There was a problem hiding this comment.
I'll look into it tonight.
|
I'm getting close to making a new release. If you want your stuff in there, I need your last tweaks :) |
Also check headers in existing csv download tests.
|
I saw that the build was failing on a strange error (timeout) so I retriggered it, after which it succeeded. I'll run it a couple of times to make sure it has not become unstable. |
|
Looks good. Thanks for your patience :) |
Adds another download link to the UI when --csv-full-history was specified
See: #1468
Does not implement the suggested 'download-all'