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.

Sign up for GitHub

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

Moved tests to the more appropriate directory and added a test case based on the performance improvement to be brought by #5427 #6094

Merged
merged 12 commits into from
Apr 24, 2024
Merged

Moved tests to the more appropriate directory and added a test case based on the performance improvement to be brought by #5427 #6094

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c639a00
Move tests to .ci, updated workflow version to accommodate that chang…
Anirban166 Apr 18, 2024
0c0ce61
Added to the path filter to trigger the action on changes to the tests
Anirban166 Apr 18, 2024
28483e8
Avoid repetition in test case titles
Anirban166 Apr 18, 2024
a59ad44
Update .github/workflows/performance-tests.yml
Anirban166 Apr 18, 2024
f4c5d28
rm whitespace
Anirban166 Apr 20, 2024
a2e9134
Titles
Anirban166 Apr 20, 2024
51e4615
Reverted whitespace changes
Anirban166 Apr 22, 2024
1d8c699
Removed the linter comment added from #5908
Anirban166 Apr 22, 2024
2b7fe3a
Merge branch 'master' into add-a-test-and-move-to-ci
MichaelChirico Apr 23, 2024
f23abda
Fixed the third test
Anirban166 Apr 23, 2024
91b5de3
Titles
Anirban166 Apr 23, 2024
6c7157d
Better wording
Anirban166 Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions inst/atime/tests.R → .ci/atime/tests.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# A function to customize R package metadata and source files to facilitate version-specific installation and testing.
#
# This is specifically tailored for handling data.table which requires specific changes in non-standard files (such as the object file name in Makevars and version checking code in onLoad.R)
# to support testing across different versions (base and HEAD for PRs, commits associated with historical regressions, etc.) of the package.
# This is specifically tailored for handling data.table which requires specific changes in non-standard files (such as the object file name in Makevars and version checking code in onLoad.R)
# to support testing across different versions (base and HEAD for PRs, commits associated with historical regressions, etc.) of the package.
# It appends a SHA1 hash to the package name (PKG.SHA), ensuring each version can be installed and tested separately.
#
# @param old.Package Current name of the package.
Expand Down Expand Up @@ -29,7 +29,7 @@ pkg.edit.fun = function(old.Package, new.Package, sha, new.pkg.path) {
Package_ <- gsub(".", "_", old.Package, fixed = TRUE)
new.Package_ <- paste0(Package_, "_", sha)
pkg_find_replace(
"DESCRIPTION",
"DESCRIPTION",
paste0("Package:\\s+", old.Package),
paste("Package:", new.Package))
pkg_find_replace(
Expand All @@ -55,13 +55,13 @@ pkg.edit.fun = function(old.Package, new.Package, sha, new.pkg.path) {
}

# A list of performance tests.
#
#
# Each entry in this list corresponds to a performance test and contains a sublist with three mandatory arguments:
# - N: A numeric sequence of data sizes to vary.
# - setup: An expression evaluated for every data size before measuring time/memory.
# - expr: An expression that will be evaluated for benchmarking performance across different git commit versions.
# - expr: An expression that will be evaluated for benchmarking performance across different git commit versions.
Anirban166 marked this conversation as resolved.
Show resolved Hide resolved
# This must call a function from data.table using a syntax with double or triple colon prefix.
# The package name before the colons will be replaced by a new package name that uses the commit SHA hash.
# The package name before the colons will be replaced by a new package name that uses the commit SHA hash.
# (For instance, data.table:::[.data.table will become data.table.some_40_digit_SHA1_hash:::[.data.table)
#
# Optional parameters that may be useful to configure tests:
Expand All @@ -70,10 +70,11 @@ pkg.edit.fun = function(old.Package, new.Package, sha, new.pkg.path) {
# - sha.vec: Named character vector or a list of vectors that specify data.table-specific commit SHAs for testing across those different git commit versions.
# For historical regressions, use 'Before', 'Regression', and 'Fixed' (otherwise something like 'Slow' or 'Fast' ideally).
# @note Please check https://github.com/tdhock/atime/blob/main/vignettes/data.table.Rmd for more information.
# nolint start: undesirable_operator_linter. ':::' needed+appropriate here.
test.list <- list(
# Performance regression discussed in: https://github.com/Rdatatable/data.table/issues/4311
# Performance regression discussed in: https://github.com/Rdatatable/data.table/issues/4311
# Fixed in: https://github.com/Rdatatable/data.table/pull/4440
"Test regression fixed in #4440" = list(
"Regression fixed in #4440" = list(
pkg.edit.fun = pkg.edit.fun,
N = 10^seq(3,8),
setup = quote({
Expand All @@ -88,8 +89,8 @@ test.list <- list(

# Test based on: https://github.com/Rdatatable/data.table/issues/5424
# Performance regression introduced from a commit in: https://github.com/Rdatatable/data.table/pull/4491
# Fixed in: https://github.com/Rdatatable/data.table/pull/5463
"Test regression fixed in #5463" = list(
# Fixed in: https://github.com/Rdatatable/data.table/pull/5463
"Regression fixed in #5463" = list(
pkg.edit.fun = pkg.edit.fun,
N = 10^seq(3, 8),
setup = quote({
Expand All @@ -101,8 +102,20 @@ test.list <- list(
key = "g")
dt_mod <- copy(dt)
}),
expr = quote(data.table:::`[.data.table`(dt_mod, , N := .N, by = g)),
expr = quote(data.table:::`[.data.table`(dt_mod, , N := .N, by = g)),
Before = "be2f72e6f5c90622fe72e1c315ca05769a9dc854", # Parent of the regression causing commit (https://github.com/Rdatatable/data.table/commit/e793f53466d99f86e70fc2611b708ae8c601a451) in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits)
Regression = "e793f53466d99f86e70fc2611b708ae8c601a451", # Commit responsible for regression in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits)
Fixed = "58409197426ced4714af842650b0cc3b9e2cb842") # Last commit in the PR that fixed the regression (https://github.com/Rdatatable/data.table/pull/5463/commits)
Fixed = "58409197426ced4714af842650b0cc3b9e2cb842"), # Last commit in the PR that fixed the regression (https://github.com/Rdatatable/data.table/pull/5463/commits)
Copy link
Member

Choose a reason for hiding this comment

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

Another aside: @tdhock let's support test.list having a NULL final entry, that way we can have like:

test.list <- list(
  "A" = list(...),
  "B" = list(...),
  NULL
)

And avoid the pesky git diff on irrelevant lines every time a test is added (as in here, where we add , --> diff)

Copy link
Member

Choose a reason for hiding this comment

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

great idea

Copy link
Member

Choose a reason for hiding this comment

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

implemented in atime github, https://github.com/tdhock/atime/blob/ea591e37a8fbf69a06a0cdbb8a51108cb73290fb/R/test.R#L14 I will file a follow up PR next week after that gets on CRAN


# Issue reported in: https://github.com/Rdatatable/data.table/issues/5426
# To be fixed in: https://github.com/Rdatatable/data.table/pull/5427
"Improvement implemented in #5427" = list(
pkg.edit.fun = pkg.edit.fun,
N = 10^seq(1, 7),
setup = quote({
DT = replicate(N, 1, simplify = FALSE)
}),
expr = quote(data.table:::setDT(DT)),
Slow = "c4a2085e35689a108d67dacb2f8261e4964d7e12", # Parent of the first commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/commit/7cc4da4c1c8e568f655ab5167922dcdb75953801)
Fast = "1872f473b20fdcddc5c1b35d79fe9229cd9a1d15") # Last commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/pull/5427/commits)
)
5 changes: 3 additions & 2 deletions .github/workflows/performance-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ on:
- synchronize
paths:
- 'R/**'
- 'src/**'
- 'src/**'
- '.ci/atime/**'

jobs:
comment:
Expand All @@ -20,4 +21,4 @@ jobs:
GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
repo_token: ${{ secrets.GITHUB_TOKEN }}
steps:
- uses: Anirban166/Autocomment-atime-results@v1.1.6
- uses: Anirban166/Autocomment-atime-results@v1.2.0
Loading