Skip to content

Conversation

@ben-schwen
Copy link
Member

Towards #6400

Current froll checking times would not pass CRAN.

Change is structured in such a way that releases do not run froll.Rraw. One can still force the execution by setting Sys.setenv("RunAllDataTableTests"="yes")

What still needs to be discussed is whether we want to disable main test suite on CRAN...

@codecov
Copy link

codecov bot commented Nov 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.07%. Comparing base (df7fa80) to head (d6c1411).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7429      +/-   ##
==========================================
- Coverage   99.13%   99.07%   -0.07%     
==========================================
  Files          85       85              
  Lines       16618    16609       -9     
==========================================
- Hits        16474    16455      -19     
- Misses        144      154      +10     

☔ 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.

@jangorecki
Copy link
Member

jangorecki commented Nov 15, 2025

Why disable the whole test script. I think that all non-batch tests should finish under 10s and they are very useful, as show by recently detected breaking changes detected by revdeps check.

Copy link
Member

@jangorecki jangorecki left a comment

Choose a reason for hiding this comment

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

Marking as request change ls because we want to have at least the basic tests run each time. Only batch testing are the issue here. So either a branching them based on env car, or interactive(), or moving to another test script seems much better way to address the issue.

Copy link
Member

@jangorecki jangorecki left a comment

Choose a reason for hiding this comment

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

Thank you for enabling tests back.
As for env bars, we, and base R always uses capital case with underscores. Any good reason to use camel case instead?

@ben-schwen
Copy link
Member Author

Thank you for enabling tests back. As for env bars, we, and base R always uses capital case with underscores. Any good reason to use camel case instead?

Nah that was just shamelessly copied from Dirk.

Should we maybe run batch tests only in Gitlab and RMD-check-occasional?

@github-actions
Copy link

  • HEAD=deactivate_tests_on_cran slower P<0.001 for memrecycle regression fixed in #5463
  • HEAD=deactivate_tests_on_cran slower P<0.001 for setDT improved in #5427
    Comparison Plot

Generated via commit d6c1411

Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 4 minutes and 54 seconds
Installing different package versions 10 minutes and 25 seconds
Running and plotting the test cases 2 minutes and 58 seconds

@Rdatatable Rdatatable deleted a comment from jan-swissre Nov 18, 2025
@jangorecki
Copy link
Member

I would run them as well for interactive session. Maybe we can detect interactive inside test.data.table() and set env var there?
The purpose is to avoid it on CRAN only. We could avoid it as well in --as-cran CI jobs, so we have a workflow that can be checked to confirm if the mechanism works as expected.

@ben-schwen
Copy link
Member Author

ben-schwen commented Nov 18, 2025

I would run them as well for interactive session. Maybe we can detect interactive inside test.data.table() and set env var there? The purpose is to avoid it on CRAN only.

We could but if you run them interactively you gonna type test.data.table(script="frollBatch.Rraw") anyway, hence leaving the optional to its default?

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.

4 participants