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

37 independent snapshot testing for tlf ae specificr #75

Closed
wants to merge 7 commits into from

Conversation

howardbaik
Copy link
Contributor

Snapshot testing for tlf_ae_specific(). All tests passed

@howardbaik howardbaik linked an issue Jul 31, 2022 that may be closed by this pull request
@howardbaik howardbaik requested a review from LittleBeannie July 31, 2022 23:52
@elong0527
Copy link
Collaborator

Please follow the update in #73 to pass code coverage test.

@howardbaik howardbaik changed the title 37 independent testing for tlf ae specificr 37 independent snapshot testing for tlf ae specificr Aug 2, 2022
@howardbaik
Copy link
Contributor Author

Do you know why we keep on getting Rplots.pdf when we run tests for tlf_ae_summary() and tlf_ae_specific()?

@LittleBeannie
Copy link
Collaborator

Rplots.pdf

I am not sure if this is helpful or not. An answer from google: https://stackoverflow.com/questions/6535927/how-do-i-prevent-rplots-pdf-from-being-generated

@elong0527
Copy link
Collaborator

Do you know why we keep on getting Rplots.pdf when we run tests for tlf_ae_summary() and tlf_ae_specific()?

please remove it. That is a default behavior when you have a graph device open. But not sure how to avoid that~

@howardbaik
Copy link
Contributor Author

howardbaik commented Aug 3, 2022

Do you know why we keep on getting Rplots.pdf when we run tests for tlf_ae_summary() and tlf_ae_specific()?

please remove it. That is a default behavior when you have a graph device open. But not sure how to avoid that~

The SO post suggested including pdf(NULL), so I included this at the top of my testing script, and it seems to have solved the problem.

@elong0527
Copy link
Collaborator

Do you know why we keep on getting Rplots.pdf when we run tests for tlf_ae_summary() and tlf_ae_specific()?

please remove it. That is a default behavior when you have a graph device open. But not sure how to avoid that~

The SO post suggested including pdf(NULL), so I included this at the top of my testing script, and it seems to have solved the problem.

Cool, my solution is to update .gitignore to avoid unnecessary files.

@fb-elong
Copy link
Contributor

fb-elong commented Aug 3, 2022

Let me commit a few changes to reduce the commit size and pass the test.

@fb-elong
Copy link
Contributor

fb-elong commented Aug 3, 2022

I guess the change in #77 is sufficient to pass the test.

@elong0527
Copy link
Collaborator

Fixed the testing in #78. To enhance robustness, let's snapshot tbl instead of the rtf file.

@elong0527 elong0527 closed this Aug 4, 2022
@howardbaik
Copy link
Contributor Author

Got it. Thanks for the help @elong0527

@BrianLang
Copy link
Collaborator

BrianLang commented Aug 17, 2022

Fixed the testing in #78. To enhance robustness, let's snapshot tbl instead of the rtf file.

This is an approach that is quite dangerous, tlf_ae_specific doesn't process the tbl enough to make this test meaningful, or am I missing something? This is probably why we missed the tlf_ae_specific was still needing half of its development.

@elong0527, as a compromise, could we snapshot the header and first 5 lines of the tlf?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Independent Testing for tlf_ae_specific.R
5 participants