Skip to content
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

Fix failed pipeline #285

Merged
merged 6 commits into from
Aug 29, 2024
Merged

Fix failed pipeline #285

merged 6 commits into from
Aug 29, 2024

Conversation

donyunardi
Copy link
Contributor

Fixes #284

Small note, I ended up using testthat::skip_if_not_installed("ggplot2") at the beginning of several unit test files (e.g., test-DownloadReportModule.R, test-LoadReporterModule.R, etc.) because they depend on ggplot2 at the start of the test.

I don't think this update will affect our code coverage since code coverage is done using a different workflow (Check) which I believe will install all dependencies, including ggplot2.

@donyunardi donyunardi linked an issue Aug 27, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Aug 27, 2024

✅ All contributors have signed the CLA
Posted by the CLA Assistant Lite bot.

@donyunardi
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

github-actions bot commented Aug 27, 2024

badge

Code Coverage Summary

Filename                  Stmts    Miss  Cover    Missing
----------------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------
R/AddCardModule.R           146       2  98.63%   170, 207
R/ContentBlock.R             18       2  88.89%   57-63
R/DownloadModule.R          238      67  71.85%   98-104, 152, 183-188, 197-202, 205-210, 219-224, 227-232, 240-245, 248-253, 260-265, 268-273, 312-316
R/FileBlock.R                13       0  100.00%
R/LoadReporterModule.R      103      19  81.55%   100-105, 108-113, 119-124, 136
R/NewpageBlock.R              2       0  100.00%
R/PictureBlock.R             30       2  93.33%   20, 118
R/Previewer.R               372      95  74.46%   96-98, 101-102, 184-213, 217-219, 222, 289, 304, 306-309, 312, 315-323, 437-481
R/RcodeBlock.R               15       0  100.00%
R/Renderer.R                113      37  67.26%   97-112, 216, 224, 233, 235-256
R/ReportCard.R               84       3  96.43%   239, 244, 269
R/Reporter.R                107       6  94.39%   273-278
R/ResetModule.R              53       0  100.00%
R/SimpleReporter.R           32       0  100.00%
R/TableBlock.R                9       0  100.00%
R/TextBlock.R                13       0  100.00%
R/utils.R                   126      86  31.75%   7, 38-97, 99, 102-109, 137, 161-169, 206-215
R/yaml_utils.R               81       2  97.53%   78, 289
R/zzz.R                      14      10  28.57%   2-13, 19
TOTAL                      1569     331  78.90%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: d71c75d

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Aug 27, 2024

Unit Tests Summary

  1 files   18 suites   33s ⏱️
189 tests 189 ✅ 0 💤 0 ❌
329 runs  329 ✅ 0 💤 0 ❌

Results for commit d71c75d.

♻️ This comment has been updated with latest results.

@donyunardi
Copy link
Contributor Author

@m7pr
Copy link
Contributor

m7pr commented Aug 28, 2024

ggplot2 is in Suggests so such a workaround with skip_if_not_installed() is recommended.

Few points:

  • I don't understand the addition of rmarkdown to VignetteBuilder honestly and for now I don't think it should be there.
  • @donyunardi should we rewrite few tests so that they use regular base plots, so that we don't skip them?

@m7pr
Copy link
Contributor

m7pr commented Aug 28, 2024

Or is rmarkdown here just to satisfy the noSuggest check where rmarkdown was not found and when it's in VignetteBuilder then it's found? If this is the only way to do that, then this is fine

R/Reporter.R Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Aug 28, 2024

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
TableBlock 💚 $13.81$ $-1.12$ $0$ $0$ $0$ $0$

Results for commit 34c6428

♻️ This comment has been updated with latest results.

@donyunardi
Copy link
Contributor Author

donyunardi commented Aug 28, 2024

Or is rmarkdown here just to satisfy the noSuggest check where rmarkdown was not found and when it's in VignetteBuilder then it's found? If this is the only way to do that, then this is fine

Yes, @pawelru did the research here

@donyunardi should we rewrite few tests so that they use regular base plots, so that we don't skip them?

That was my first impression too, but we're only skipping this for workflows like noSuggest. Since we have other workflows with all dependencies installed, these tests are still being executed.

If we want to upgrade these test cases to use regular base plots, I support it, but I'd prefer to handle it outside of this issue since it would require more attention.

@donyunardi donyunardi merged commit b7ad603 into main Aug 29, 2024
30 checks passed
@donyunardi donyunardi deleted the 284-fix-failed-pipeline branch August 29, 2024 14:39
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix failed pipeline
3 participants