Integration tests for `/api/v2/chat/completions-detection` #360

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

Jump to bottom

Merged

declark1 merged 2 commits into foundation-model-stack:main from pmcjr:unary_chat_completions_detection

Apr 4, 2025

Collaborator

pmcjr commented Apr 4, 2025

This PR addresses #341.

The list of implemented test cases can be found on Integration tests for /api/v2/chat/completions-detection endpoint #341 (comment) and were implemented on tests/chat_completions_detection.rs.


          ✅ Adding integration test cases for unary chat completions

e8076d2

Signed-off-by: Paulo Caldeira <[email protected]>

pmcjr requested review from gkumbhat, evaline-ju and declark1 as code owners

April 4, 2025 12:24

pmcjr changed the title ~~✅ Adding integration test cases for unary chat compl…~~ Integration tests for /api/v2/chat/completions-detection

pmcjr requested a review from mdevino

April 4, 2025 13:10

mdevino suggested changes

View reviewed changes

Collaborator

mdevino left a comment

Thanks for adding the tests. I've left a few suggestions.

tests/common/orchestrator.rs

               pub const ORCHESTRATOR_DETECTION_ON_GENERATION_ENDPOINT: &str = "/api/v2/text/detection/generated";
               pub const ORCHESTRATOR_CONTEXT_DOCS_DETECTION_ENDPOINT: &str = "/api/v2/text/detection/context";
               pub const ORCHESTRATOR_CHAT_DETECTION_ENDPOINT: &str = "/api/v2/text/detection/chat";
+              pub const ORCHESTRATOR_CHAT_COMPLETIONS_DETECTION_ENDPOINT: &str =

Collaborator

mdevino Apr 4, 2025

nit: Line break, so it lines up with the endpoints grouping in the public API.

Suggested change

      
            pub const ORCHESTRATOR_CHAT_COMPLETIONS_DETECTION_ENDPOINT: &str =
          
            pub const ORCHESTRATOR_CHAT_COMPLETIONS_DETECTION_ENDPOINT: &str =

Collaborator Author

pmcjr Apr 4, 2025

Applying suggestion

tests/chat_completions_detection.rs Outdated

+              // Constants
+              const CHUNKER_NAME_SENTENCE: &str = "sentence_chunker";
+              const MODEL_ID: &str = "my-super-model-8B";
+              const CHAT_COMPLETIONS_ENDPOINT: &str = "/v1/chat/completions";

Collaborator

mdevino Apr 4, 2025

I'm understanding this is the chat_generation server endpoint. If so, I'd recommend moving anything related to this server into a new file tests/orchestrator/chat_generation.rs to line up with the current test code architecture.

Collaborator Author

pmcjr Apr 4, 2025

Yeah it is, I'll add the chat endpoint /v1/chat/completions under tests/common/chat_completions.rs to keep inline with architecture.

tests/chat_completions_detection.rs Outdated

+                              stream: false,
+                              ..Default::default()
+                          });
+                      then.json(chat_completions_response.clone());

Collaborator

mdevino Apr 4, 2025

This mocktail method also accepts references so you can avoid the unnecessary clone.

Suggested change

      
                    then.json(chat_completions_response.clone());
          
                    then.json(&chat_completions_response);

tests/chat_completions_detection.rs Outdated

+                  assert_eq!(results.choices[0], chat_completions_response.choices[0]);
+                  assert_eq!(results.choices[1], chat_completions_response.choices[1]);
+                  assert_eq!(results.warnings, vec![]);
+                  assert_eq!(results.detections, expected_detections.clone());

Collaborator

mdevino Apr 4, 2025

Unneeded clone as expected_detections will not be used after this line.

Suggested change

      
                assert_eq!(results.detections, expected_detections.clone());
          
                assert_eq!(results.detections, expected_detections);

tests/chat_completions_detection.rs Outdated

+              // Validates that requests with input detector configured returns detections
+              #[test(tokio::test)]
+              async fn input_detections() -> Result<(), anyhow::Error> {
+                  let detector_name = DETECTOR_NAME_ANGLE_BRACKETS_WHOLE_DOC;

Collaborator

mdevino Apr 4, 2025

I'd recommend using a detector with a different chunker in this test to make sure chunker calls are working as expected.

Collaborator Author

pmcjr Apr 4, 2025

Changed to sentence_chunker for both input and output detection

tests/chat_completions_detection.rs Outdated

+                              stream: false,
+                              ..Default::default()
+                          });
+                      then.json(chat_completions_response.clone());

Collaborator

mdevino Apr 4, 2025

Suggested change

      
                    then.json(chat_completions_response.clone());
          
                    then.json(&chat_completions_response);

tests/chat_completions_detection.rs Outdated

+              // Validates that requests with output detector configured returns detections
+              #[test(tokio::test)]
+              async fn output_detections() -> Result<(), anyhow::Error> {
+                  let detector_name = DETECTOR_NAME_ANGLE_BRACKETS_WHOLE_DOC;

Collaborator

mdevino Apr 4, 2025

I'd recommend using a different chunker to make sure chunker calls are working as expected.

Collaborator Author

pmcjr Apr 4, 2025

Changed to sentence_chunker for both input and output detection

tests/chat_completions_detection.rs Outdated

+                              contents: vec![output_text.into()],
+                              detector_params: DetectorParams::new(),
+                          });
+                      then.json(vec![expected_detections.clone()]);

Collaborator

mdevino Apr 4, 2025

Suggested change

      
                    then.json(vec![expected_detections.clone()]);
          
                    then.json(vec![&expected_detections]);

tests/chat_completions_detection.rs Outdated

+                              stream: false,
+                              ..Default::default()
+                          });
+                      then.json(chat_completions_response.clone());

Collaborator

mdevino Apr 4, 2025

Suggested change

      
                    then.json(chat_completions_response.clone());
          
                    then.json(&chat_completions_response);

tests/chat_completions_detection.rs

+              // Validate that invalid orchestrator requests returns 422 error
+              #[test(tokio::test)]
+              async fn orchestrator_validation_error() -> Result<(), anyhow::Error> {

Collaborator

mdevino Apr 4, 2025

I would add more scenarios for this tests, such as making sure missing mandatory fields return 422, but I believe this could be done in a future PR.

Collaborator Author

pmcjr Apr 4, 2025

Yes, we are validating unknown extra fields here but surely we could add more scenarios for this test, such as suggested missing mandatory fields.


          🚧 Apply suggestions from code review

1e04cc0

Signed-off-by: Paulo Caldeira <[email protected]>

declark1 approved these changes

View reviewed changes

declark1 merged commit 49af6ea into foundation-model-stack:main

2 checks passed

mdevino mentioned this pull request

Integration tests for /api/v2/chat/completions-detection endpoint #341

Closed

2 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mdevino mdevino requested changes

declark1 declark1 approved these changes

gkumbhat Awaiting requested review from gkumbhat gkumbhat is a code owner

evaline-ju Awaiting requested review from evaline-ju evaline-ju is a code owner

Labels

None yet