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

Adding complete tests for wrapping in pagination and export #113

Merged
merged 6 commits into from
May 2, 2023

Conversation

Melkiades
Copy link
Contributor

Fixes #111 and #112

@Melkiades Melkiades added bug Something isn't working enhancement New feature or request sme labels Apr 12, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 12, 2023

badge

Code Coverage Summary

Filename                 Stmts    Miss  Cover    Missing
---------------------  -------  ------  -------  -------------------------------
R/listing_export.R          58      11  81.03%   97, 132, 140-154
R/paginate_listing.R       103       5  95.15%   93, 98, 102, 105, 108
R/rlistings_methods.R      100      14  86.00%   24-25, 39, 54, 137-140, 227-233
R/rlistings.R              130      12  90.77%   294-297, 301-304, 332-335
TOTAL                      391      42  89.26%

Diff against main

Filename                 Stmts    Miss  Cover
---------------------  -------  ------  -------
R/rlistings_methods.R       +1      -1  +1.15%
TOTAL                       +1      -1  +0.28%

Results for commit: 18fa71a

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Apr 12, 2023

Unit Tests Summary

  1 files    4 suites   39s ⏱️
19 tests 19 ✔️ 0 💤 0
60 runs  60 ✔️ 0 💤 0

Results for commit a93c65d.

♻️ This comment has been updated with latest results.

@gmbecker
Copy link
Collaborator

gmbecker commented Apr 13, 2023

Fixes #111 and #112

This PR does not appear to fix #112 (and related #114), because it only changes tests...

@Melkiades
Copy link
Contributor Author

Fixes #111 and #112

This PR does not appear to fix #112 (and related #114), because it only changes tests...

Still working on it! sorry, it should have been a draft atm

@Melkiades Melkiades marked this pull request as draft April 13, 2023 07:45
@Melkiades Melkiades marked this pull request as ready for review April 14, 2023 15:00
@Melkiades
Copy link
Contributor Author

@gmbecker why alias is necessary?

@gmbecker
Copy link
Collaborator

gmbecker commented Apr 14, 2023

@gmbecker why alias is necessary?

I'm not completely sure what you're asking here, but if it is waht I think you might be saying, its to prevent what I described here: #112 (comment)

The problem is that unlike TableTree objects in rtables, listing_dfs inherit from a class that the default toString method (thinks it) can process. So we need to prevent that.

@Melkiades Melkiades requested review from edelarua and removed request for edelarua and insights-engineering-bot April 18, 2023 10:47
R/rlistings_methods.R Outdated Show resolved Hide resolved
@Melkiades Melkiades requested review from gmbecker and removed request for gmbecker and edelarua April 24, 2023 13:58
R/rlistings_methods.R Outdated Show resolved Hide resolved
@Melkiades Melkiades requested review from gmbecker and shajoezhu and removed request for gmbecker April 28, 2023 14:56
@Melkiades
Copy link
Contributor Author

It is complete now @shajoezhu @gmbecker

Copy link
Collaborator

@shajoezhu shajoezhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant! Thanks @Melkiades

@Melkiades Melkiades merged commit e1c7f70 into main May 2, 2023
@Melkiades Melkiades deleted the 111_add_tests_wrapping@main branch May 2, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request sme
Projects
None yet
3 participants