这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@jberryman
Copy link
Collaborator

@jberryman jberryman commented Sep 5, 2019

We add a new pytest flag --accept that will automatically write back
yaml files with updated responses. This makes it much easier and less
error-prone to update test cases when we expect output to change, or
when authoring new tests.

Second we make sure to test that we actually preserve the order of the
selection set when returning results. This is a "SHOULD" part of the
spec but seems pretty important and something that users will rely on.

To support both of the above we use ruamel.yaml which preserves a
certain amount of formatting and comments (so that --accept can work in
a failry ergonomic way), as well as ordering (so that when we write yaml
the order of keys has meaning that's preserved during parsing).

Use ruamel.yaml everywhere for consistency (since both libraries have
different quirks).

Quirks of ruamel.yaml:

UPDATE: marked xfail for the failing case we found case: #3271 . This isn't great since we lose the other functionality those tests were excercising.

@jberryman
Copy link
Collaborator Author

This exposes some potential bugs and inconsistencies that still need to be investigated.

@netlify
Copy link

netlify bot commented Sep 5, 2019

Deploy preview for hasura-docs ready!

Built with commit 008a3df

https://deploy-preview-2831--hasura-docs.netlify.com

@hasura-bot
Copy link
Contributor

Review app for commit c94fbb1 deployed to Heroku: https://hge-ci-pull-2831.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2831-c94fbb18

@nizar-m
Copy link
Contributor

nizar-m commented Sep 5, 2019

Awesome feature --accept. Would love to see this merged.

@jberryman
Copy link
Collaborator Author

TODO I should strip trailing whitespace from the graphql query string before writing back, so ruamel doesn't do ugly formatting.

@hasura-bot
Copy link
Contributor

Review app for commit d713c6a deployed to Heroku: https://hge-ci-pull-2831.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2831-d713c6aa

@hasura-bot
Copy link
Contributor

Review app for commit 8f48bd3 deployed to Heroku: https://hge-ci-pull-2831.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2831-8f48bd35

@jberryman jberryman force-pushed the brandon-test-yaml-fixups branch from 8f48bd3 to 0c7d8e3 Compare October 30, 2019 13:39
@hasura-bot
Copy link
Contributor

Review app for commit 0c7d8e3 deployed to Heroku: https://hge-ci-pull-2831.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2831-0c7d8e3f

@jberryman jberryman force-pushed the brandon-test-yaml-fixups branch from 0c7d8e3 to 67f74ca Compare October 30, 2019 16:05
@hasura-bot
Copy link
Contributor

Review app for commit 67f74ca deployed to Heroku: https://hge-ci-pull-2831.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2831-67f74ca6

@jberryman jberryman marked this pull request as ready for review October 30, 2019 16:58
@jberryman jberryman changed the title WIP: Test result ordering, add --accept test mode to automatically accept new/changed test cases Test result ordering, add --accept test mode to automatically accept new/changed test cases Oct 30, 2019
lexi-lambda
lexi-lambda previously approved these changes Nov 1, 2019
Copy link
Contributor

@lexi-lambda lexi-lambda left a comment

Choose a reason for hiding this comment

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

This LGTM. Thanks for doing this!

#!/usr/bin/env python3

# This module is for tests that validate our tests or test framework, make sure
# tests are running correctly, or test our python test helpers.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 This is a nice detail.

@hasura-bot
Copy link
Contributor

Review app for commit af547ae deployed to Heroku: https://hge-ci-pull-2831.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2831-af547ae1

@jberryman
Copy link
Collaborator Author

Cool, thanks! (I don't have permissions to merge to master; let me know if there's anything else you need from me)

See: https://stackoverflow.com/q/57682657/176841

I don't think this is something we care about, and we need to fix this
for ruamel which uses the more sane v1.2 spec.
…t changed test cases

We add a new pytest flag `--accept` that will automatically write back
yaml files with updated responses. This makes it much easier and less
error-prone to update test cases when we expect output to change, or
when authoring new tests.

Second we make sure to test that we actually preserve the order of the
selection set when returning results. This is a "SHOULD" part of the
spec but seems pretty important and something that users will rely on.

To support both of the above we use ruamel.yaml which preserves a
certain amount of formatting and comments (so that --accept can work in
a failry ergonomic way), as well as ordering (so that when we write yaml
the order of keys has meaning that's preserved during parsing).

Use ruamel.yaml everywhere for consistency (since both libraries have
different quirks).

Quirks of ruamel.yaml:
- trailing whitespace in multiline strings in yaml files isn't written
  back out as we'd like: https://bitbucket.org/ruamel/yaml/issues/47/multiline-strings-being-changed-if-they
- formatting is only sort of preserved; ruamel e.g. normalizes
  indentation. Normally the diff is pretty clean though, and you can
  always just check in portions of your test file after --accept

fixup
These were generated with `--accept` and inspected individually.

Mark failing cases as xfail: hasura#3271
@hasura-bot
Copy link
Contributor

Review app for commit 008a3df deployed to Heroku: https://hge-ci-pull-2831.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2831-008a3df3

@lexi-lambda lexi-lambda merged commit 70a1ca6 into hasura:master Nov 5, 2019
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-2831.herokuapp.com is deleted

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.

4 participants