use full demo data in test to avoid problem using new demo data #521
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Presumably, these lines restricted the number of lines of demo data being used to improve performance (?), but this makes the test very sensitive to legitimate changes in the demo datasets, e.g. if the explicitly specified rows no longer contain matching rows. An incoming update to the demo datasets in RMI-PACTA/r2dii.data#316 will make this test fail, but this PR will allow it to pass by removing the explicit filtering of rows. https://github.com/RMI-PACTA/r2dii.plot/blob/b4b87e8a3fe0a07367b1de15ef097250507f258e/tests/testthat/test-market_share.R#L43-L44
cjyetman
requested review from
gbdubs,
jdhoffa,
MonikaFu,
jacobvjk and
AlexAxthelm
and removed request for
gbdubs
jacobvjk
approved these changes
Nov 3, 2023
AlexAxthelm
approved these changes
Nov 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Presumably, these lines restricted the number of lines of demo data being used to improve performance (?), but this makes the test very sensitive to legitimate changes in the demo datasets, e.g. if the explicitly specified rows no longer contain matching rows. An incoming update to the demo datasets in RMI-PACTA/r2dii.data#316 will make this test fail, but this PR will allow it to pass by removing the explicit filtering of rows.
r2dii.plot/tests/testthat/test-market_share.R
Lines 43 to 44 in b4b87e8
EDIT: also had to update a snapshot to take into account a newly added (by rlang developers, unrelated to this PR)
<rlang_message>tag so that tests here passEDIT: tests still fail because apparently existing release version of
r2dii.data::abcd_demoyearcolumn is type<double>instead of<integer>even though the script to create it explicitly sets it to<integer>. Tempted to just let that slide here because the new version ofr2dii.data::abcd_demowill be properly typed and that test will then pass.