Skip to content

Conversation

@IndrajeetPatil
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2021

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.02%. Comparing base (2002162) to head (8b76c8c).
⚠️ Report is 960 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #856   +/-   ##
=======================================
  Coverage   90.02%   90.02%           
=======================================
  Files          47       47           
  Lines        2486     2486           
=======================================
  Hits         2238     2238           
  Misses        248      248           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 4f9b7b2 is merged into master:

  •   :ballot_box_with_check:cache_applying: 34.3ms -> 35.8ms [-0.74%, +9.79%]
  •   :ballot_box_with_check:cache_recording: 1.58s -> 1.58s [-1.16%, +0.86%]
  •   :ballot_box_with_check:without_cache: 4.19s -> 4.18s [-1.37%, +0.79%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil
Copy link
Collaborator Author

@lorenzwalthert Let me know if you need me to make any changes for this one.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Oct 26, 2021

I am no longer sure we want to solve it with a test (since people with older r versions will always have this problem) and I’d do the trat the other way round (skip if R >= 4.2 to make it fail in the future).

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if d53d104 is merged into master:

  •   :ballot_box_with_check:cache_applying: 30.5ms -> 30.5ms [-1.21%, +1.22%]
  •   :ballot_box_with_check:cache_recording: 1.22s -> 1.22s [-0.74%, +1.1%]
  •   :ballot_box_with_check:without_cache: 3.18s -> 3.18s [-0.71%, +0.68%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil
Copy link
Collaborator Author

So now the test will be skipped from R 4.2 onwards.
Once it is released, we can change the test and run it only for these newer versions.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if f1dbbb3 is merged into master:

  •   :ballot_box_with_check:cache_applying: 34.6ms -> 35ms [-4.02%, +6.37%]
  •   :ballot_box_with_check:cache_recording: 1.55s -> 1.55s [-1.28%, +1.54%]
  •   :ballot_box_with_check:without_cache: 4.02s -> 4.03s [-0.94%, +1.26%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

Sorry I need to think about this. Thanks for your time, I will either merge it or solve the bug later.

@IndrajeetPatil
Copy link
Collaborator Author

Yepp, no worries. Feel free to also close this PR if you think this is not the optimal solution to handle this issue right now.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 67a413e is merged into master:

  •   :ballot_box_with_check:cache_applying: 32.6ms -> 32.4ms [-5.37%, +4.07%]
  •   :ballot_box_with_check:cache_recording: 1.45s -> 1.46s [-1.69%, +2.23%]
  •   :ballot_box_with_check:without_cache: 3.88s -> 3.82s [-3.39%, +0.01%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil
Copy link
Collaborator Author

Another possibility is to use snapshot tests, with a different snap for each R version:

image

@lorenzwalthert
Copy link
Collaborator

@IndrajeetPatil there is #783 for testthat 3e, but I don't think we need snaps for making the test R version dependent. Superseded by #883.

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.

3 participants