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.

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

Fix and test schema generation interface #220

Merged
merged 20 commits into from Mar 22, 2019

Conversation

pmantica1
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage increased (+0.2%) to 93.68% when pulling cfd45c8 on update-schema-generation-documentation into e24b4eb on master.

@bojanserafimov
Copy link
Collaborator

This api has no test coverage. We didn't know that type_id is not the correct name of the argument. Maybe add a test with some small made up schema? For your convenience, here's the input I used that exposed this bug:

    schema_graph = SchemaGraph([
        {
            'name': 'Entity',
            'abstract': True,
            'superClasses': [],
            'properties': [
                {
                    'name': 'name',
                    'type': schema_properties.PROPERTY_TYPE_STRING_ID,
                }
            ]
        },
        {
            'name': 'Animal',
            'abstract': False,
            'superClasses': ['Entity'],
            'properties': [
                {
                    'name': 'net_worth',
                    'type': schema_properties.PROPERTY_TYPE_DECIMAL_ID,
                }
            ]
        },
        {
            'name': 'Location',
            'abstract': False,
            'superClasses': ['Entity'],
            'properties': [
                {
                    'name': 'description',
                    'type': schema_properties.PROPERTY_TYPE_STRING_ID,
                }
            ]
        },
    ])

describes the name of an
endpoint of the edge.
- defaultValue: string, the textual representation of the
default value for the property, as
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the visual indentation is off by one space (compare string vs default)

},
]
schema, _ = get_graphql_schema_from_orientdb_schema_data(orientdb_schema_data)
self.assertMatchSnapshot(print_schema(schema))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need this to be a snapshot test here? This feels like a unit test to me since we have well-defined input and output, and a single function call being tested. Invoking all the snapshot testing machinery feels like overkill for this use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a good idea because every time I've made a small change to the way we generate the schema, I've had to print it and then copy paste it into test_helpers. If I introduce this test as an unit test, every time we change the schema we now have to change two things.

@@ -64,6 +64,8 @@ CREATE INDEX Animal.net_worth NOTUNIQUE
CREATE CLASS Animal_ParentOf EXTENDS E
CREATE PROPERTY Animal_ParentOf.in LINK Animal
CREATE PROPERTY Animal_ParentOf.out LINK Animal
ALTER CLASS Animal_ParentOf CUSTOM human_name_in="Parent"
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test on the generated schema graph that these properties exist and have their expected values?

@obi1kenobi
Copy link
Contributor

This also doesn't merge cleanly anymore, so I'll hold off on reviewing until you've had a chance to resolve the merge conflicts.

@pmantica1 pmantica1 force-pushed the update-schema-generation-documentation branch from 1c69a18 to 4ff3b15 Compare March 22, 2019 13:19
@pmantica1 pmantica1 force-pushed the update-schema-generation-documentation branch from dd4a8c6 to 190dd1c Compare March 22, 2019 13:27
@pmantica1 pmantica1 force-pushed the update-schema-generation-documentation branch from 190dd1c to c39de5b Compare March 22, 2019 13:31
@pmantica1 pmantica1 changed the title Update schema generation docs Fix and test schema generation interface Mar 22, 2019
@obi1kenobi obi1kenobi merged commit 7c70f2e into master Mar 22, 2019
@obi1kenobi obi1kenobi deleted the update-schema-generation-documentation branch March 22, 2019 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants