Repository navigation
[MAINT] test html output - #6179
Conversation
|
👋 @Remi-Gau Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6179 +/- ##
==========================================
- Coverage 95.53% 94.28% -1.25%
==========================================
Files 307 307
Lines 43491 43496 +5
Branches 4417 4417
==========================================
- Hits 41548 41010 -538
- Misses 1304 1825 +521
- Partials 639 661 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@hndgzkn Only done for some function and one masker report for now but it could be extended. Let me know what you think. |
hndgzkn
left a comment
There was a problem hiding this comment.
I've put some minor comments, otherwise I think it seems to be very helpful.
Shall we do it for all types of maskers, fitted and not fitted, I am not sure?
There was a problem hiding this comment.
Would it be possible to scroll the file and save the following parts as well?
There was a problem hiding this comment.
Well I wanted to try something different which was to make the height of the report higher, but it seems we cannot resize reports.
It's a bug and I opened an issue about it.
Once it is fixed, we should indeed cover the whole height of the report.
There was a problem hiding this comment.
It's a bug and I opened an issue about it.
I can have a look at it if that's ok with you.
There was a problem hiding this comment.
yes thanks: on that note probably worth making the report_dims of the generate_report of GLM a common parameters to all reports
I am not 100% sure but I suspect that making this resizing work will require some HTML and CSS tweaking: just a heads up
There was a problem hiding this comment.
@hndgzkn I actually managed to change the size of the viewport in the generated figures so we should be getting the entire reports now
There was a problem hiding this comment.
Nice solution, the images are very good now.
I would start with the fitted ones. Once we have things fixed for the report height I would add some others. |
| """Use as a decorator to skip tests requiring memory_profiler.""" | ||
|
|
||
| def dummy_func(): | ||
| import pytest |
There was a problem hiding this comment.
moved the pytest import local to make it possible to import this module even when pytest is not installed
| @@ -0,0 +1,16 @@ | |||
| const template = require('./template.js') | |||
|
|
|||
| const VIEWPORT = { x: 0, y: 0, width: 1200, height: 750 } | |||
There was a problem hiding this comment.
May be height could be bigger for maskers as well, non? Some seem to be cut.
There was a problem hiding this comment.
yeah one of the report would then fails because if we display the whole report then there is file path displayed and obviously not the same when run locally and online so I as voluntarily keeping the viewport smaller.
|
I am going to go ahead and merge this one as it is a bit of a blocker for many other things. |
Changes proposed in this pull request: