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

schema tests #695

Open
wants to merge 8 commits into
base: development
Choose a base branch
from
Open

schema tests #695

wants to merge 8 commits into from

Conversation

A-Harby
Copy link
Contributor

@A-Harby A-Harby commented Jan 30, 2024

Description

Add tests for the schema checking server error, response code, content type, response schema, and response headers against the proxy schema itself.

Changes

Add a tests.py file, requirements.txt, and workflow.yml.

Related Issues

#51

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring

@A-Harby A-Harby marked this pull request as ready for review January 30, 2024 15:25
@Omarabdul3ziz
Copy link
Contributor

Good job ya @A-Harby
I have some comments, as far as I understood schemathesis reads the openapi spec and try to make each call to the server.

  • we have more live proxy servers than the ones running on *.grid.tf and i don't see a need to test them all actually tests will fail cause it reads the specs from the pushed code which may contains some changes not deployed yet on the running server, so i guess it will be enough to test on push/pull-request for the server running from the code itself. what do you think?
  • also, i see there is already an action from schemathsis themselves https://github.com/schemathesis/action, did you consider using it?

@A-Harby
Copy link
Contributor Author

A-Harby commented Apr 15, 2024

  • we have more live proxy servers than the ones running on *.grid.tf and i don't see a need to test them all actually tests will fail cause it reads the specs from the pushed code which may contains some changes not deployed yet on the running server, so i guess it will be enough to test on push/pull-request for the server running from the code itself. what do you think?

I added the trigger on the PR on the main branch and just left the manual trigger for the test run on every network (to make sure everything is working fine).

That does need a subscription, and running the coding does pretty much the same.

@Omarabdul3ziz
Copy link
Contributor

correct me if I got it wrong, schematest checks the openapi spec file and do all the possible requests to the target servers.
it is frequently that we update these specs like adding new filters or changing the response schemes.
the code will read the specs from the pushed code - which will contain the changes - but will try to make the request to the running server - which doesn't have the changes yet - so it will fail.

isn't it enough to run the test once on the push/pr? if we do need to run it on all the servers can we get the specs for each server running? maybe from https://gridproxy.dev.grid.tf/swagger/doc.json instead of reading it from the codebase

@A-Harby
Copy link
Contributor Author

A-Harby commented Apr 17, 2024

isn't it enough to run the test once on the push/pr? if we do need to run it on all the servers can we get the specs for each server running? maybe from https://gridproxy.dev.grid.tf/swagger/doc.json instead of reading it from the codebase

I did that in 1811cac, the PR should be ready for review.

@Omarabdul3ziz
Copy link
Contributor

code looks good to me, but the test fails due to some needed updates in the schema. we need to take care of this before merge this pr.
i will try to resolve them or you @A-Harby are very welcome to help :D

Copy link
Member

@sameh-farouk sameh-farouk left a comment

Choose a reason for hiding this comment

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

@Omarabdul3ziz Any schema or API fixes are outside the scope of this PR, so we can merge it (since it's working as expected).

We should create new issues/PRs for the discovered issues.

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

3 participants