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

Jump to bottom

fix: input types loose out on provided graphQL documentation #1657

Closed
wants to merge 3 commits into from
+55 −4
Closed

fix: input types loose out on provided graphQL documentation #1657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc7223b
Add descriptions to input object types
sn99 Apr 4, 2024
fc1e604
Add descriptions to input object types
sn99 Apr 4, 2024
1a5a548
Merge remote-tracking branch 'origin/bug-1625' into bug-1625
sn99 Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/config/config.rs
View file
Open in desktop
Expand Up @@ -835,4 +835,30 @@ mod tests {
)]);
assert_eq!(actual, expected);
}

#[test]
fn test_from_sdl_description() {
let actual = Config::from_sdl(
r#"
"""
Some Documentation
"""
input Foo {
id: Int!
}
"#,
)
.to_result()
.unwrap();

let expected = Config::default().types(vec![(
"Foo",
Type {
doc: Some("Some Documentation".to_string()),
..Default::default()
}
.fields(vec![("id", Field::int().required(true))]),
)]);
assert_eq!(actual, expected);
}
Comment on lines +838 to +863
Copy link
Contributor

@coderabbitai coderabbitai bot Apr 4, 2024 •
edited

Choose a reason for hiding this comment

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

Add assertions for the fields and scalar properties in the test_from_sdl_description test to ensure comprehensive coverage.

#[test]
fn test_from_sdl_description() {
    // Test setup and execution...
    assert_eq!(actual.types["Foo"].fields["id"].type_of, "Int");
    assert!(!actual.types["Foo"].scalar);
}

This enhancement will verify that the input object type Foo not only has the correct documentation but also accurately reflects its fields and scalar property, ensuring a more thorough test coverage.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop this test and add one like this https://github.com/tailcallhq/tailcall/tree/main/tests/execution

Copy link
Contributor

Choose a reason for hiding this comment

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

@tusharmath, based on your request to drop the existing test and add a new one similar to those in the tests/execution directory, here's a proposed approach:

  1. Remove the existing test: The existing test_from_sdl_description test in src/config/config.rs will be removed as per your instruction.

  2. Add a new test: A new test will be added following the structure and style observed in the tests/execution directory. The provided output from the tests/execution directory suggests a pattern of testing involving server configurations, mock requests, and assertions. However, since the task is to modify a Rust test file, and the provided examples are not directly applicable to Rust code, I'll generate a Rust test that conceptually aligns with the pattern of setting up a scenario, executing a function, and asserting the outcome.

Here's a conceptual Rust test that aligns with the instructions:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_new_sdl_description_behavior() {
        // Setup the test environment
        let sdl = r#"
            """
            Example Documentation
            """
            input ExampleInput {
                id: Int!
            }
        "#;

        // Execute the function under test
        let config = Config::from_sdl(sdl).to_result().unwrap();

        // Assert the expected outcome
        assert_eq!(config.types["ExampleInput"].doc, Some("Example Documentation".to_string()));
        assert_eq!(config.types["ExampleInput"].fields["id"].type_of, "Int");
        assert!(config.types["ExampleInput"].fields["id"].required);
    }
}

This test checks that the SDL description is correctly parsed and associated with the ExampleInput type, and that the id field is correctly identified as an Int and marked as required. This aligns with the objective of ensuring that descriptions for input object types are correctly parsed and included in the generated schema, as well as verifying the field types and requirements.

Please review this approach and let me know if it meets your expectations or if further adjustments are needed.

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# List the files in the tests/execution directory to choose a representative file.
fd . tests/execution --exec cat {}

Length of output: 180053

Copy link
Author

Choose a reason for hiding this comment

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

@tusharmath I am having trouble with what the test will look like, can you give me a hint?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor

Choose a reason for hiding this comment

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

Check out this test — tests/execution/test-query-documentation.md

Copy link
Author

Choose a reason for hiding this comment

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

@tusharmath does the test look alright?

}
15 changes: 11 additions & 4 deletions src/config/from_document.rs
View file
Open in desktop
Expand Up @@ -156,9 +156,12 @@ fn to_types(
)
.some(),
TypeKind::Enum(enum_type) => Valid::succeed(Some(to_enum(enum_type))),
TypeKind::InputObject(input_object_type) => {
to_input_object(input_object_type, &type_definition.node.directives).some()
}
TypeKind::InputObject(input_object_type) => to_input_object(
input_object_type,
&type_definition.node.description,
&type_definition.node.directives,
)
.some(),
TypeKind::Union(_) => Valid::none(),
TypeKind::Scalar => Valid::succeed(Some(to_scalar_type())),
}
Expand Down Expand Up @@ -241,11 +244,15 @@ fn to_enum(enum_type: EnumType) -> config::Type {
}
fn to_input_object(
input_object_type: InputObjectType,
description: &Option<Positioned<String>>,
directives: &[Positioned<ConstDirective>],
) -> Valid<config::Type, String> {
to_input_object_fields(&input_object_type.fields)
.fuse(Protected::from_directives(directives.iter()))
.map(|(fields, protected)| config::Type { fields, protected, ..Default::default() })
.map(|(fields, protected)| {
let doc = description.to_owned().map(|pos| pos.node);
config::Type { fields, protected, doc, ..Default::default() }
})
}

fn to_fields_inner<T, F>(
Expand Down
18 changes: 18 additions & 0 deletions tests/execution/test-input-documentation.md
View file
Open in desktop
@@ -0,0 +1,18 @@
---
check_identity: true
---

# test-input-documentation

```graphql @server
schema @server @upstream(baseURL: "http://jsonplacheholder.typicode.com") {
input: Foo
}

"""
This is a test
"""
input Foo {
id: Int!
}
```