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

chore: add integration tests for CORS #1989

Closed
wants to merge 24 commits into from

Conversation

varshith257
Copy link
Contributor

@varshith257 varshith257 commented May 20, 2024

Summary:

  1. The set_common_headers function has been created to handle common header settings to remove code duplication patterns.
  2. Added Integration tests for CORS .

Issue Reference(s):

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added the type: fix Iterations on existing features or infrastructure. label May 20, 2024
@varshith257 varshith257 changed the title fix: enchance request_handler.rs and added cors integration tests fix: enchanced request_handler.rs and added cors integration tests May 20, 2024
@varshith257
Copy link
Contributor Author

@tusharmath I think it's ready for your review. If any suggestions or modifications needed, I am ready to incorporate them

tests/core/cors.rs Outdated Show resolved Hide resolved
@tusharmath tusharmath marked this pull request as draft May 23, 2024 07:32
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 25, 2024
@varshith257
Copy link
Contributor Author

Will update the tests as required soon

@github-actions github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 25, 2024
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels May 27, 2024
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels May 29, 2024
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 31, 2024
@github-actions github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jun 4, 2024
Copy link

github-actions bot commented Jun 6, 2024

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels Jun 6, 2024
@tusharmath tusharmath changed the title fix: enchanced request_handler.rs and added cors integration tests chore: add integration tests for CORS Jun 11, 2024
req_ctx: &RequestContext,
app_ctx: &AppContext,
) {
set_common_headers(resp.headers_mut(), app_ctx, req_ctx);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes doesn't seem to be doing much. Can we revert it to the one it was in main?

Copy link
Contributor Author

@varshith257 varshith257 Jun 11, 2024

Choose a reason for hiding this comment

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

Okay! @tusharmath.

@github-actions github-actions bot added the type: chore Routine tasks like conversions, reorganization, and maintenance work. label Jun 11, 2024
@varshith257 varshith257 requested a review from tusharmath June 11, 2024 12:18
@varshith257
Copy link
Contributor Author

varshith257 commented Jun 11, 2024

@tusharmath Any tip would be great for my efforts of trying to get optimise the request_handler.rs file from a long ? and thanks for guiding to make contributions to tailcall

@tusharmath tusharmath enabled auto-merge (squash) June 12, 2024 05:46
Copy link
Contributor

@tusharmath tusharmath left a comment

Choose a reason for hiding this comment

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

Fix review comments. Whenever completed, please mark it as ready to review.

@tusharmath tusharmath marked this pull request as draft June 12, 2024 05:48
auto-merge was automatically disabled June 12, 2024 05:48

Pull request was converted to draft

@varshith257 varshith257 marked this pull request as ready for review June 12, 2024 12:47
@varshith257
Copy link
Contributor Author

@tusharmath I have added some more tests. PTAL

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.15%. Comparing base (c1dd7c7) to head (c205d4a).

Current head c205d4a differs from pull request most recent head 40f5e90

Please upload reports for the commit 40f5e90 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1989      +/-   ##
==========================================
- Coverage   85.91%   84.15%   -1.76%     
==========================================
  Files         211      212       +1     
  Lines       19993    19987       -6     
==========================================
- Hits        17177    16821     -356     
- Misses       2816     3166     +350     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@varshith257
Copy link
Contributor Author

I am unable to run cargo test to generate snapshots. This emitting lot of errors

@varshith257 varshith257 requested a review from tusharmath June 12, 2024 12:50
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jun 17, 2024
@github-actions github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jun 19, 2024
@server(
port: 8000
headers: {
cors: {allowHeaders: ["*"], allowMethods: ["GET", "POST", "OPTIONS"], allowOrigins: ["https://tailcall.run"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is intended to test headers that put automatically for tailcall.run and therefore we don't need to specify headers in test - we need to test that it works without headers

}

type Query {
example: String @http(path: "/example")
Copy link
Contributor

Choose a reason for hiding this comment

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

need to mock this request

Comment on lines +106 to +109
expected:
status: 200
headers:
access-control-allow-origin: https://tailcall.run
Copy link
Contributor

Choose a reason for hiding this comment

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

we use snapshots and not explicit expected definitions in tests. That should be removed and proper snapshots should be generated

@meskill
Copy link
Contributor

meskill commented Jun 21, 2024

Closing for now since it requires additional work. Feel free to reopen this when it's ready. If you have any troubles with code/tooling please ask in our #contributors channel on Discord

@meskill meskill closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore Routine tasks like conversions, reorganization, and maintenance work. type: fix Iterations on existing features or infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: add tests for Tailcall CORS
3 participants