Skip to content

feat(java, scala): added logic for finding test data in root and parent dir of project #824

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

Merged
yangxk1 merged 3 commits into from
Jan 12, 2026
Merged

feat(java, scala): added logic for finding test data in root and parent dir of project #824

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
612adf3
added logic for finding test data in root and parent dir of project f…
shivendra-dev54 Jan 9, 2026
757c4a9
try to fix
Jan 11, 2026
204e3aa
added directory checks and updated error messages
shivendra-dev54 Jan 12, 2026
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
20 changes: 19 additions & 1 deletion maven-projects/java/src/test/java/org/apache/graphar/graphinfo/GraphInfoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,25 @@
import org.junit.Test;

public class GraphInfoTest {
public static final String root = System.getenv("GAR_TEST_DATA") + "/java";
private static String resolveTestData() {
String path = System.getenv("GAR_TEST_DATA");
if (path == null) {
path = System.getProperty("gar.test.data");
}
if (path == null) {
String[] candidates = {"../../testing", "../testing", "testing"};
for (String p : candidates) {
java.io.File dir = new java.io.File(p).getAbsoluteFile();
if (new java.io.File(dir, "ldbc_sample/csv/ldbc_sample.graph.yml").exists()) {
return dir.getAbsolutePath();
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The validation logic is inconsistent with the existing implementation in TestUtil.java. The current code only checks if the file exists, but TestUtil.java also verifies that the directory exists and is actually a directory. This could lead to false positives if the parent path doesn't exist. Consider adding an additional check to verify that the parent directory exists and is a directory before checking for the specific file.

Suggested change
if (new java.io.File(dir, "ldbc_sample/csv/ldbc_sample.graph.yml").exists()) {
return dir.getAbsolutePath();
if (dir.exists() && dir.isDirectory()) {
java.io.File graphFile =
new java.io.File(dir, "ldbc_sample/csv/ldbc_sample.graph.yml");
if (graphFile.exists()) {
return dir.getAbsolutePath();
}

Copilot uses AI. Check for mistakes.
}
}
throw new IllegalStateException("GAR_TEST_DATA not found");
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The exception type differs from the one used in TestUtil.java, which uses RuntimeException. Consider using RuntimeException instead of IllegalStateException for consistency across the codebase, or document why a different exception type is appropriate here.

Suggested change
throw new IllegalStateException("GAR_TEST_DATA not found");
throw new RuntimeException("GAR_TEST_DATA not found");

Copilot uses AI. Check for mistakes.
}
return path;
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The validation approach differs from TestUtil.java which performs additional verification after finding a path. TestUtil.java re-validates the chosen path to ensure both the directory and the marker file exist. This double-checking approach is more robust and catches edge cases. Consider adding a final validation step after selecting a path to ensure it's valid before returning it.

Suggested change
return dir.getAbsolutePath();
}
}
throw new IllegalStateException("GAR_TEST_DATA not found");
}
return path;
path = dir.getAbsolutePath();
break;
}
}
if (path == null) {
throw new IllegalStateException("GAR_TEST_DATA not found");
}
}
java.io.File baseDir = new java.io.File(path).getAbsoluteFile();
java.io.File markerFile =
new java.io.File(baseDir, "ldbc_sample/csv/ldbc_sample.graph.yml");
if (!baseDir.isDirectory() || !markerFile.exists()) {
throw new IllegalStateException("GAR_TEST_DATA not found");
}
return baseDir.getAbsolutePath();

Copilot uses AI. Check for mistakes.
}

public static final String root = resolveTestData() + "/java";

@Test
public void test1() {
Expand Down
21 changes: 18 additions & 3 deletions maven-projects/spark/graphar/src/test/scala/org/apache/graphar/BaseTestSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,25 @@ abstract class BaseTestSuite extends AnyFunSuite with BeforeAndAfterAll {
var spark: SparkSession = _

override def beforeAll(): Unit = {
if (System.getenv("GAR_TEST_DATA") == null) {
throw new IllegalArgumentException("GAR_TEST_DATA is not set")
def resolveTestData(): String = {
Option(System.getenv("GAR_TEST_DATA"))
.orElse(Option(System.getProperty("gar.test.data")))
.getOrElse {
val candidates = Seq("../../testing", "../testing", "testing")
candidates
.map(p => new java.io.File(p).getAbsoluteFile)
.find(d =>
new java.io.File(d, "ldbc_sample/csv/ldbc_sample.graph.yml")
.exists()
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The validation logic is inconsistent with the existing implementation in TestUtil.java. The Scala code only checks if the file exists, but TestUtil.java also verifies that the directory exists and is actually a directory. This could lead to false positives if the parent path doesn't exist. Consider adding an additional check to verify that the parent directory exists and is a directory before checking for the specific file.

Suggested change
new java.io.File(d, "ldbc_sample/csv/ldbc_sample.graph.yml")
.exists()
d.exists() && d.isDirectory &&
new java.io.File(d, "ldbc_sample/csv/ldbc_sample.graph.yml")
.isFile

Copilot uses AI. Check for mistakes.
)
.map(_.getAbsolutePath)
.getOrElse(
throw new IllegalArgumentException("GAR_TEST_DATA not found")
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The exception type differs from the one used in TestUtil.java, which uses RuntimeException. Consider using RuntimeException instead of IllegalArgumentException for consistency across the codebase, or document why a different exception type is appropriate here.

Suggested change
throw new IllegalArgumentException("GAR_TEST_DATA not found")
throw new RuntimeException("GAR_TEST_DATA not found")

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The error message is less informative than the one used in TestUtil.java. TestUtil.java provides a more detailed message that explains both the environment variable option and the directory structure requirement. Consider updating the message to match the clarity of TestUtil.java's error message which states: "GAR_TEST_DATA not found or invalid. Please set GAR_TEST_DATA environment variable to point to the testing directory or ensure the testing directory exists with ldbc_sample/csv/ldbc_sample.graph.yml".

Suggested change
throw new IllegalArgumentException("GAR_TEST_DATA not found")
throw new IllegalArgumentException(
"GAR_TEST_DATA not found or invalid. Please set GAR_TEST_DATA environment variable to point to the testing directory or ensure the testing directory exists with ldbc_sample/csv/ldbc_sample.graph.yml"
)

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@yangxk1 yangxk1 Jan 12, 2026

Choose a reason for hiding this comment

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

@shivendra-dev54, You can consider adopting this suggestion, it makes the error message clearer

)
}
}
testData = System.getenv("GAR_TEST_DATA")

testData = resolveTestData()
Comment on lines +32 to +67
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

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

The validation approach differs from TestUtil.java which performs additional verification after finding a path. TestUtil.java re-validates the chosen path to ensure both the directory and the marker file exist. This double-checking approach is more robust and catches edge cases. Consider adding a final validation step after selecting a path to ensure it's valid before returning it.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@yangxk1 yangxk1 Jan 12, 2026

Choose a reason for hiding this comment

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

@shivendra-dev54 Please consider this comment, it looks good

spark = SparkSession
.builder()
.enableHiveSupport()
Expand Down
Loading