[wip] use native annotation tags #75
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request modifies the GitHub Actions workflow for testing Scala and Python, focusing on specific test execution commands in several jobs. It introduces a new Java file defining three annotations for tagging tests, updates existing test classes to use these annotations instead of a previous tagging method, and corrects minor comments in the test files. Additionally, a trait used for conditional test execution has been removed. Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
tchow-zlai
force-pushed
the
tchow/shard-tests
branch
from
f53cb2c to
e86d571
Compare
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.ElementType; | ||
|
|
||
| @org.scalatest.TagAnnotation |
There was a problem hiding this comment.
@piyush-zlai wdyt about this? Reason for the change is that this more natively leverages the scalatest TagAnnotation framework and allows us to do exclusions on the commandline without additional code ie:
sbt "spark/testOnly -- -l ai.chronon.spark.test.JoinTestTag"
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/test_scala_and_python.yaml (1)
86-87: Update step names to reflect specific test categories.The step names in these jobs say "Run other spark tests" but they are actually running specific tagged tests (join, mutations, and fetcher tests respectively). Consider updating the names for clarity:
- name: Run other spark tests + name: Run join spark tests- name: Run other spark tests + name: Run mutations spark tests- name: Run other spark tests + name: Run fetcher spark testsAlso applies to: 105-106, 124-125
spark/src/test/scala/ai/chronon/spark/test/FetcherTest.scala (1)
Line range hint
141-144: Consider enhancing method documentation.The test helper methods (
generateMutationData,generateRandomData,generateEventOnlyData) would benefit from additional documentation explaining:
- Input parameters and their impact
- Expected test data patterns
- Use cases for different test scenarios
Also applies to: 234-237, 467-470
spark/src/test/scala/ai/chronon/spark/test/JoinTest.scala (1)
43-43: Consider removing unused imports if flagged by static analysis.The import statement includes
Ignorewhich might not be used in this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.github/workflows/test_scala_and_python.yaml(3 hunks)spark/src/test/java/ai/chronon/spark/test/Tags.java(1 hunks)spark/src/test/scala/ai/chronon/spark/test/FeatureWithLabelJoinTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/FetcherTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/JoinTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/MutationsTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/TaggedFilterSuite.scala(0 hunks)
💤 Files with no reviewable changes (1)
- spark/src/test/scala/ai/chronon/spark/test/TaggedFilterSuite.scala
✅ Files skipped from review due to trivial changes (2)
- spark/src/test/java/ai/chronon/spark/test/Tags.java
- spark/src/test/scala/ai/chronon/spark/test/FeatureWithLabelJoinTest.scala
🔇 Additional comments (8)
.github/workflows/test_scala_and_python.yaml (2)
87-87: LGTM! Consistent update to native annotation tags.
The changes correctly update the test tags to use fully qualified annotation class names, which aligns with the transition to native tag annotations. This change improves clarity and maintainability by explicitly referencing the annotation classes.
Also applies to: 106-106, 125-125
86-87: Verify resource allocation strategy.
There are notable differences in resource allocation between test jobs:
other_spark_testsuses ubuntu-8_cores-32_gb runner with 24G heap- These specific test jobs use ubuntu-latest with 8G heap
Let's verify if this allocation is intentional and optimal.
Also applies to: 105-106, 124-125
✅ Verification successful
Let me gather more information about the test characteristics and memory usage patterns.
Based on the gathered information, I can now provide a final response:
Memory allocation differences are justified by test characteristics
The different memory allocations between test jobs are appropriate because:
- JoinTest primarily works with in-memory data transformations and Spark operations
- The other Spark tests (using 24G heap) include more comprehensive test suites that likely process larger datasets and run multiple operations in parallel
The current allocation of 8G heap for JoinTest is sufficient as the test focuses on join operations with in-memory data structures and doesn't show evidence of heavy resource requirements in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test execution times and memory usage patterns
# Search for test execution patterns and memory requirements
rg -A 5 "testOnly.*JoinTestTag|testOnly.*MutationsTestTag|testOnly.*FetcherTestTag" --glob '!.github'
# Look for documentation about resource requirements
fd -t f "README|CONTRIBUTING" -X cat {} 2>/dev/null | grep -i "memory\|resource\|requirement"
Length of output: 2190
Script:
#!/bin/bash
# Check the test files for memory-intensive operations and data generation
ast-grep --pattern 'class JoinTest {
$$$
}'
# Look for other test configurations and memory settings
rg -l "SBT_OPTS.*-Xmx" --glob '!.github/workflows/test_scala_and_python.yaml'
# Check for any performance or resource-related comments in the test files
rg -i "memory|performance|resource|intensive" spark/src/test/scala/
Length of output: 13181
spark/src/test/scala/ai/chronon/spark/test/FetcherTest.scala (2)
67-69: LGTM! Class annotation changes align with native tag transition.
The transition to using @FetcherTestTag annotation and removal of TaggedFilterSuite extension successfully implements the native tag annotation approach.
Line range hint 825-844: Add test coverage for tag-based filtering.
Given the transition to native tag annotations, consider adding test cases that specifically verify:
- Tag-based test filtering functionality
- Tag exclusion using the
-loption - Multiple tag combinations
spark/src/test/scala/ai/chronon/spark/test/MutationsTest.scala (2)
49-49: LGTM: Test execution command updated correctly.
The comment has been updated to use the fully qualified annotation name, which aligns with the new native tag annotations approach.
52-53: LGTM: Test class properly annotated.
The @MutationsTestTag annotation has been correctly added to the test class, replacing the previous tagging mechanism.
Let's verify the annotation definition:
✅ Verification successful
Annotation is properly defined and used as expected
The verification confirms that:
- The
@MutationsTestTagis correctly defined as a Java annotation interface inspark/src/test/java/ai/chronon/spark/test/Tags.java - It's properly used in
MutationsTest.scalawith correct documentation for test execution - No unexpected or incorrect usages were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the MutationsTestTag annotation definition and usage
# Check the annotation definition
ast-grep --pattern 'class MutationsTestTag {
$$$
}'
# Check for any other usages of this tag
rg "MutationsTestTag" -A 2
Length of output: 764
spark/src/test/scala/ai/chronon/spark/test/JoinTest.scala (2)
49-49: Verify consistent annotation usage across test files.
Since this is a WIP PR transitioning to native tag annotations, ensure that:
- All test files follow the same pattern of using native annotations
- The running instructions are consistently documented
Let's check other test files for annotation usage:
✅ Verification successful
No consistency issues found with test annotations
Based on the search results, there are only a few test files using custom test tags (@JoinTestTag, @MutationsTestTag, @FetcherTestTag), while most test files use standard JUnit annotations (@Test, @Before, @After). The change to use @JoinTestTag in JoinTest.scala appears to be consistent with the existing pattern of using custom test tags where specific test categorization is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check annotation usage consistency across test files
# Look for files with @*TestTag annotations
ast-grep --pattern '@$_TestTag'
# Look for files still using the old tagging method
rg 'override def tagName'
Length of output: 61
Script:
#!/bin/bash
# Let's try a broader search for test files and their annotations
# First, find all test files
fd "Test.scala$" --type f
# Then look for any annotations in these files
rg "^[[:space:]]*@\w+" --type scala
# Also check for any imports of test-related annotations
rg "import.*Test.*Tag" --type scala
Length of output: 42368
48-50: LGTM! Clear documentation and proper annotation usage.
The comment clearly documents how to run the test with the new tag syntax, and the @JoinTestTag annotation is correctly applied at the class level.
Let's verify the annotation class exists:
Summary
For example, now we can do tag exclusions:
sbt sbt "spark/testOnly -- -l ai.chronon.spark.test.FetcherTestTag"note the
-l.Checklist
Summary by CodeRabbit
New Features
JoinTestTag,FetcherTestTag, andMutationsTestTagfor improved test categorization.Bug Fixes
FeatureWithLabelJoinTestclass for accuracy.Chores
TaggedFilterSuitetrait to streamline testing processes.