Repository navigation
[DAPS-1916] - tests add integration test for schema client handler to cmake - #1918
Merged
JoshuaSBrown merged 22 commits intoMar 28, 2026
Conversation
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…gration-unit-testing' into 1906-DAPS-feat-core-ci-test-integration-unit-testing-2
Contributor
Reviewer's GuideAdds an ArangoDB-backed integration test suite for SchemaHandler, wires it into CTest/CMake with Docker-based fixtures, enables core integration tests in the core-build Docker image, fixes a schema ID bug in the Foxx router, and adjusts build/test scripts and mocks to support the new flow. Sequence diagram for SchemaHandler integration test with ArangoDBsequenceDiagram
actor Dev
participant Docker as datafed-core-build_container
participant CTest
participant Start as start_test_arango.sh
participant Arango as ArangoDB_test_instance
participant Test as test_SchemaHandler
participant Stop as stop_test_arango.sh
participant Foxx as schema_router.js
Dev->>Docker: Run build/test (ctest or entrypoint)
Docker->>CTest: Invoke integration test suite
CTest->>Start: Execute
Start->>Arango: Start container and initialize DB
CTest->>Test: Run SchemaHandler integration tests
Test->>Arango: Store and retrieve schema documents
Arango->>Foxx: Call schema_router for schema operations
Foxx-->>Arango: Persist schema with id parsed.id + ":" + obj.ver
Test-->>CTest: Report test results
CTest->>Stop: Execute
Stop->>Arango: Stop and clean up test DB
CTest-->>Docker: Return overall status
Docker-->>Dev: Build/test result
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
JoshuaSBrown
added
Component: Core
JoshuaSBrown
added this to the
Create a LinkML Validator Service for DataFed Schema Support milestone
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
core/server/tests/integration/CMakeLists.txt,_arango_test_envsetsDATAFED_DATABASE_NAME=${ARANGO_TEST_DATABASE_NAME}but onlyDATAFED_TEST_DATABASE_NAMEis defined above, so this env var will be empty; it should likely reference${DATAFED_TEST_DATABASE_NAME}instead. stop_test_arango.shcurrently has the core logic that checks the state file and stops/removes the container fully commented out, which means containers started by the fixture will never be cleaned up; either restore that logic or update the fixture design to avoid leaking test containers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `core/server/tests/integration/CMakeLists.txt`, `_arango_test_env` sets `DATAFED_DATABASE_NAME=${ARANGO_TEST_DATABASE_NAME}` but only `DATAFED_TEST_DATABASE_NAME` is defined above, so this env var will be empty; it should likely reference `${DATAFED_TEST_DATABASE_NAME}` instead.
- `stop_test_arango.sh` currently has the core logic that checks the state file and stops/removes the container fully commented out, which means containers started by the fixture will never be cleaned up; either restore that logic or update the fixture design to avoid leaking test containers.
## Individual Comments
### Comment 1
<location path="core/server/tests/integration/test_SchemaHandler.cpp" line_range="381-390" />
<code_context>
+ BOOST_TEST(parsed["properties"].contains("unit"));
+}
+
+BOOST_AUTO_TEST_CASE(revise_without_def_skips_validation) {
+ REQUIRE_AVAILABLE();
+
+ std::string id = createTestSchema("test_revise_no_def");
+
+ // Revise with no def — should not throw from validation
+ SchemaReviseRequest rev_req;
+ SchemaDataReply rev_reply;
+ rev_req.set_id(id);
+ // Deliberately NOT setting def
+
+ // May throw from DB (revision logic) but must NOT throw from validation
+ try {
+ handler->handleRevise(TEST_USER, rev_req, rev_reply, log_context);
+ if (rev_reply.schema_size() > 0) {
+ created_schemas.push_back(rev_reply.schema(0).id());
+ }
+ } catch (TraceException &e) {
+ std::string msg = e.toString();
+ BOOST_TEST(msg.find("Invalid metadata schema") == std::string::npos);
+ BOOST_TEST(msg.find("Schema validation failed") == std::string::npos);
+ }
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Make the "skip validation when no def" tests assert on behavior rather than exception message content
Here you catch `TraceException` and assert that its message does not contain certain substrings. That makes the test brittle (tied to wording) and still allows non-validation failures to pass. To better express the intent that validation is skipped when `def` is absent, consider asserting that `handleRevise` does not throw in this case (or that it returns a success status / non-empty reply), and add a separate test that explicitly provokes a validation error with invalid JSON. Applying the same approach in `update_without_def_skips_validation` would make both tests more robust and behavior-focused.
Suggested implementation:
```cpp
nlohmann::json parsed =
nlohmann::json::parse(view_reply.schema(0).def());
BOOST_TEST(parsed["properties"].contains("unit"));
}
BOOST_AUTO_TEST_CASE(revise_without_def_skips_validation) {
REQUIRE_AVAILABLE();
std::string id = createTestSchema("test_revise_no_def");
// Revise with no def — validation should be skipped and must not throw
SchemaReviseRequest rev_req;
SchemaDataReply rev_reply;
rev_req.set_id(id);
// Deliberately NOT setting def
BOOST_REQUIRE_NO_THROW(
handler->handleRevise(TEST_USER, rev_req, rev_reply, log_context));
// If a new schema revision was created, track it for cleanup
if (rev_reply.schema_size() > 0) {
created_schemas.push_back(rev_reply.schema(0).id());
}
}
BOOST_AUTO_TEST_CASE(revise_with_invalid_def_fails_validation) {
REQUIRE_AVAILABLE();
std::string id = createTestSchema("test_revise_invalid_def");
SchemaReviseRequest rev_req;
SchemaDataReply rev_reply;
rev_req.set_id(id);
// Intentionally invalid JSON to trigger schema validation
rev_req.set_def("{ invalid json");
BOOST_CHECK_EXCEPTION(
handler->handleRevise(TEST_USER, rev_req, rev_reply, log_context),
TraceException,
[](const TraceException &e) {
std::string msg = e.toString();
return msg.find("Invalid metadata schema") != std::string::npos ||
msg.find("Schema validation failed") != std::string::npos;
});
}
},
"required": ["name", "value"]
})";
const std::string UPDATED_SCHEMA_DEF = R"({
"type": "object",
"properties": {
"name": { "type": "string" },
"value": { "type": "number" },
"unit": { "type": "string" },
```
1. Locate the `update_without_def_skips_validation` test in this file and adjust it analogously:
- Remove any `try`/`catch` that inspects `TraceException` messages.
- Replace it with a `BOOST_REQUIRE_NO_THROW` (or `BOOST_CHECK_NO_THROW`) around the `handleUpdate` (or equivalent) call, and keep any reply/cleanup logic intact.
2. If there is not yet a corresponding “invalid def fails validation” test for update, consider adding one similar to `revise_with_invalid_def_fails_validation` but calling the update API instead of revise.
3. If your test suite has common helpers for creating invalid schema definitions or asserting on `TraceException` content, you may want to refactor the lambda used in `BOOST_CHECK_EXCEPTION` into a shared helper to avoid duplication.
</issue_to_address>
### Comment 2
<location path="core/server/tests/integration/test_SchemaHandler.cpp" line_range="552-561" />
<code_context>
+
+BOOST_FIXTURE_TEST_SUITE(HandleSearch, ArangoFixture)
+
+BOOST_AUTO_TEST_CASE(search_finds_created_schemas) {
+ REQUIRE_AVAILABLE();
+
+ createTestSchema("test_search_a");
+ createTestSchema("test_search_b");
+ createTestSchema("test_search_c");
+
+ SchemaDataReply search_reply = waitForSearchResults("test_search_", 3);
+
+ BOOST_TEST(search_reply.schema_size() >= 3);
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen search tests by asserting on the returned schema IDs instead of only the count
Right now the test only checks `schema_size() >= 3`, which doesn't ensure the three created schemas are actually returned. Iterate over `search_reply.schema()` and assert that each expected schema (`test_search_a/b/c`) appears (e.g., by `id()` prefix or exact ID). This will catch cases where search returns the wrong or missing schemas while still meeting the count requirement.
Suggested implementation:
```cpp
BOOST_FIXTURE_TEST_SUITE(HandleSearch, ArangoFixture)
BOOST_AUTO_TEST_CASE(search_finds_created_schemas) {
REQUIRE_AVAILABLE();
createTestSchema("test_search_a");
createTestSchema("test_search_b");
createTestSchema("test_search_c");
SchemaDataReply search_reply = waitForSearchResults("test_search_", 3);
BOOST_TEST(search_reply.schema_size() >= 3);
}
```
```cpp
BOOST_AUTO_TEST_CASE(search_finds_created_schemas) {
REQUIRE_AVAILABLE();
createTestSchema("test_search_a");
createTestSchema("test_search_b");
createTestSchema("test_search_c");
SchemaDataReply search_reply = waitForSearchResults("test_search_", 3);
BOOST_TEST(search_reply.schema_size() >= 3);
bool found_a = false;
bool found_b = false;
bool found_c = false;
for (const auto& schema : search_reply.schema()) {
const auto& id = schema.id();
if (id == "test_search_a") {
found_a = true;
} else if (id == "test_search_b") {
found_b = true;
} else if (id == "test_search_c") {
found_c = true;
}
}
BOOST_TEST(found_a);
BOOST_TEST(found_b);
BOOST_TEST(found_c);
}
// Local includes
#include "client_handlers/SchemaHandler.hpp"
#include "DatabaseAPI.hpp"
#include "common/TraceException.hpp"
// Standard includes
#include <cstdlib>
#include <chrono>
#include <thread>
#include <memory>
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+381
to
+390
| BOOST_AUTO_TEST_CASE(revise_without_def_skips_validation) { | ||
| REQUIRE_AVAILABLE(); | ||
|
|
||
| std::string id = createTestSchema("test_revise_no_def"); | ||
|
|
||
| // Revise with no def — should not throw from validation | ||
| SchemaReviseRequest rev_req; | ||
| SchemaDataReply rev_reply; | ||
| rev_req.set_id(id); | ||
| // Deliberately NOT setting def |
Contributor
There was a problem hiding this comment.
suggestion (testing): Make the "skip validation when no def" tests assert on behavior rather than exception message content
Here you catch TraceException and assert that its message does not contain certain substrings. That makes the test brittle (tied to wording) and still allows non-validation failures to pass. To better express the intent that validation is skipped when def is absent, consider asserting that handleRevise does not throw in this case (or that it returns a success status / non-empty reply), and add a separate test that explicitly provokes a validation error with invalid JSON. Applying the same approach in update_without_def_skips_validation would make both tests more robust and behavior-focused.
Suggested implementation:
nlohmann::json parsed =
nlohmann::json::parse(view_reply.schema(0).def());
BOOST_TEST(parsed["properties"].contains("unit"));
}
BOOST_AUTO_TEST_CASE(revise_without_def_skips_validation) {
REQUIRE_AVAILABLE();
std::string id = createTestSchema("test_revise_no_def");
// Revise with no def — validation should be skipped and must not throw
SchemaReviseRequest rev_req;
SchemaDataReply rev_reply;
rev_req.set_id(id);
// Deliberately NOT setting def
BOOST_REQUIRE_NO_THROW(
handler->handleRevise(TEST_USER, rev_req, rev_reply, log_context));
// If a new schema revision was created, track it for cleanup
if (rev_reply.schema_size() > 0) {
created_schemas.push_back(rev_reply.schema(0).id());
}
}
BOOST_AUTO_TEST_CASE(revise_with_invalid_def_fails_validation) {
REQUIRE_AVAILABLE();
std::string id = createTestSchema("test_revise_invalid_def");
SchemaReviseRequest rev_req;
SchemaDataReply rev_reply;
rev_req.set_id(id);
// Intentionally invalid JSON to trigger schema validation
rev_req.set_def("{ invalid json");
BOOST_CHECK_EXCEPTION(
handler->handleRevise(TEST_USER, rev_req, rev_reply, log_context),
TraceException,
[](const TraceException &e) {
std::string msg = e.toString();
return msg.find("Invalid metadata schema") != std::string::npos ||
msg.find("Schema validation failed") != std::string::npos;
});
}
},
"required": ["name", "value"]
})";
const std::string UPDATED_SCHEMA_DEF = R"({
"type": "object",
"properties": {
"name": { "type": "string" },
"value": { "type": "number" },
"unit": { "type": "string" },
- Locate the
update_without_def_skips_validationtest in this file and adjust it analogously:- Remove any
try/catchthat inspectsTraceExceptionmessages. - Replace it with a
BOOST_REQUIRE_NO_THROW(orBOOST_CHECK_NO_THROW) around thehandleUpdate(or equivalent) call, and keep any reply/cleanup logic intact.
- Remove any
- If there is not yet a corresponding “invalid def fails validation” test for update, consider adding one similar to
revise_with_invalid_def_fails_validationbut calling the update API instead of revise. - If your test suite has common helpers for creating invalid schema definitions or asserting on
TraceExceptioncontent, you may want to refactor the lambda used inBOOST_CHECK_EXCEPTIONinto a shared helper to avoid duplication.
Comment on lines
+552
to
+561
| BOOST_AUTO_TEST_CASE(search_finds_created_schemas) { | ||
| REQUIRE_AVAILABLE(); | ||
|
|
||
| createTestSchema("test_search_a"); | ||
| createTestSchema("test_search_b"); | ||
| createTestSchema("test_search_c"); | ||
|
|
||
| SchemaDataReply search_reply = waitForSearchResults("test_search_", 3); | ||
|
|
||
| BOOST_TEST(search_reply.schema_size() >= 3); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen search tests by asserting on the returned schema IDs instead of only the count
Right now the test only checks schema_size() >= 3, which doesn't ensure the three created schemas are actually returned. Iterate over search_reply.schema() and assert that each expected schema (test_search_a/b/c) appears (e.g., by id() prefix or exact ID). This will catch cases where search returns the wrong or missing schemas while still meeting the count requirement.
Suggested implementation:
BOOST_FIXTURE_TEST_SUITE(HandleSearch, ArangoFixture)
BOOST_AUTO_TEST_CASE(search_finds_created_schemas) {
REQUIRE_AVAILABLE();
createTestSchema("test_search_a");
createTestSchema("test_search_b");
createTestSchema("test_search_c");
SchemaDataReply search_reply = waitForSearchResults("test_search_", 3);
BOOST_TEST(search_reply.schema_size() >= 3);
}
BOOST_AUTO_TEST_CASE(search_finds_created_schemas) {
REQUIRE_AVAILABLE();
createTestSchema("test_search_a");
createTestSchema("test_search_b");
createTestSchema("test_search_c");
SchemaDataReply search_reply = waitForSearchResults("test_search_", 3);
BOOST_TEST(search_reply.schema_size() >= 3);
bool found_a = false;
bool found_b = false;
bool found_c = false;
for (const auto& schema : search_reply.schema()) {
const auto& id = schema.id();
if (id == "test_search_a") {
found_a = true;
} else if (id == "test_search_b") {
found_b = true;
} else if (id == "test_search_c") {
found_c = true;
}
}
BOOST_TEST(found_a);
BOOST_TEST(found_b);
BOOST_TEST(found_c);
}
// Local includes
#include "client_handlers/SchemaHandler.hpp"
#include "DatabaseAPI.hpp"
#include "common/TraceException.hpp"
// Standard includes
#include <cstdlib>
#include <chrono>
#include <thread>
#include <memory>
…handler-to-cmake' of github.com:ORNL/DataFed into 1916-DAPS-tests-add-integration-test-for-schema-client-handler-to-cmake
JoshuaSBrown
changed the title
…handler-to-cmake' of github.com:ORNL/DataFed into 1916-DAPS-tests-add-integration-test-for-schema-client-handler-to-cmake
nedvedba
approved these changes
Mar 27, 2026
nedvedba
added a commit
that referenced
this pull request
Mar 31, 2026
* refactor: core server files refactored to support re-use in the mock implementation * refactor: changed mock core server to work with proto3 and reusue abstractions from core server. * fix: prevent defaults being set to undefined, and interpret numbers a… (#1861) * fix: prevent defaults being set to undefined, and interpret numbers and enums as strings. * chore: Auto-format JavaScript files with Prettier * fix: version numbers from proto3 messages follow camel case. (#1868) * docs: fix jsdoc error. * [DAPS-1862] - fix allocation change failure (#1864) * [DAPS-1663] Adding LogContext to dbGetRaw, Correlation_ID to dbMaintenance, metricThread and task_worker (#1885) Co-authored-by: Joshua S Brown <joshbro42867@yahoo.com> Co-authored-by: Joshua S Brown <brownjs@ornl.gov> * fix: mock_core server build (#1890) * [DAPS-1887] - refactor: facility fuse, remove dead code. (#1888) * [DAPS-1857] - feature: python client, tests, support schema functions, to hit feature parity with web ser… (#1859) * [DAPS-1855] - feature, core, add schema factory to decouple schema logic (#1891) * [DAPS-1896] - feature: foxx, schema format and type added to schema create API (#1897) * [DAPS-1893] - feature: common, proto3 add fields for schema type, format, and metadata format (#1894) * [DAPS-1830-1] - core foxx refactored json schema integration 1 (#1892) * [DAPS-1857-2] python client schema support (#1895) Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * [DAPS-1830-2] - core foxx refactored json schema integration (#1899) * [DAPS-1902] - core, common, feat: get schema api client into compliance with API spec file. (#1903) * [DAPS-1906-1] - feature: add core unit testing to CI (#1907) Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * [DAPS-1830-3] - core foxx refactored json schema integration 3 1827 (#1898) * [DAPS-1910] - upgrade: playwright 1.51.1 version. (#1911) * [DAPS-1914] - bug, web schema id name version mismatch (#1915) * [DAPS-1913] - refactor: foxx ci test scripts consolidate database name and default to different database for tests (#1917) * [DAPS-1916] - tests add integration test for schema client handler to cmake (#1918) Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> --------- Co-authored-by: JoshuaSBrown <brownjs@ornl.gov> Co-authored-by: Joshua S Brown <joshbro42867@yahoo.com> Co-authored-by: Austin Hampton <44103380+megatnt1122@users.noreply.github.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
JoshuaSBrown
added a commit
that referenced
this pull request
Apr 1, 2026
* [DAPS-1663] - feature: core, LogContext to dbGetRaw, Correlation_ID to dbMaintenance, metricThread and task_worker (#1885) * [DAPS-1890] - fix: test, mock_core server build (#1890) * [DAPS-1887] - refactor: facility fuse, remove dead code. (#1888) * [DAPS-1857] - feature: python client, tests, support schema functions, to hit feature parity with web ser… (#1859) * [DAPS-1855] - feature, core, add schema factory to decouple schema logic (#1891) * [DAPS-1896] - feature: foxx, schema format and type added to schema create API (#1897) * [DAPS-1893] - feature: common, proto3 add fields for schema type, format, and metadata format (#1894) * [DAPS-1830-1] - refactor: core foxx refactored json schema integration 1 (#1892) * [DAPS-1857-2] - feature: python client schema support (#1895) * [DAPS-1830-2] - refactor: core foxx refactored json schema integration (#1899) * [DAPS-1902] - feature: core, common, get schema api client into compliance with API spec file. (#1903) * [DAPS-1906-1] - feature: add core unit testing to CI (#1907) * [DAPS-1830-3] - refactor: core foxx refactored json schema integration 3 1827 (#1898) * [DAPS-1910] - upgrade: playwright 1.51.1 version. (#1911) * [DAPS-1914] - fix: web, bug in schema id name version mismatch (#1915) * [DAPS-1913] - refactor: foxx ci test scripts consolidate database name and default to different database for tests (#1917) * [DAPS-1916] - tests: add integration test for schema client handler to cmake (#1918) * [DAPS-1912] - refactor: register linkml storage engine with schema handler (#1912) * [DAPS-1919] - feature: python, web support linkml schema (#1921) * [DAPS-1923] - update: version numbers bumped. (#1923) Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Co-authored-by: Austin Hampton <amh107@latech.edu> Co-authored-by: Blake Nedved <blakeanedved@gmail.com> Co-authored-by: Polina Shpilker <infinite.loopholes@gmail.com> Co-authored-by: JoshuaSBrown <brownjs@ornl.gov>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add ArangoDB-backed integration tests for the SchemaHandler and enable running integration tests in the core Docker build.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: