Skip to content

Conversation

@Priyanshutyagiji
Copy link

No description provided.

Copy link
Contributor

@elanlaw1206 elanlaw1206 left a comment

Choose a reason for hiding this comment

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

HI Priyanshu,

Thanks for adding auth/security test coverage , the intent and scope look good.

A few things to address before merge:

  1. package.json: the "test" script is replaced from the existing Mocha wildcard to a single Jest test file. This may stop other tests from running and affect CI coverage. Suggest keeping the current "test" command and adding a separate script (e.g. "test:auth"), or handling Jest migration in a dedicated PR.

  2. auth.test.js: BASE_URL is hardcoded to http://localhost:80, which is fragile for CI and local runs. Please switch to process.env.BASE_URL with a fallback, or ideally use supertest(app) by importing the Express app directly.

  3. auth.test.js: several assertions allow 404 for protected endpoints. Allowing 404 can hide missing routes and create false positives in security tests. Suggest narrowing expected status codes to 401/403 (and 400 where applicable).

Once these are updated, I’m happy to re-review and approve.

King Hei

@elanlaw1206
Copy link
Contributor

Hi Priyanshu,

Thanks for the update, appreciate the work on this 👍

Since this PR includes changes to core files (e.g., package.json and security-related logic), I’m taking a safety-first approach to ensure it doesn’t affect the main runtime or CI behaviour.

Could you please share the test evidence showing everything still works as expected?
For example:

CI run link (green checks), and

The test command you ran locally with a brief output/result

Once confirmed and all checks remain green, I’m happy to approve and move this forward. Thanks!

King Hei

@Priyanshutyagiji
Copy link
Author

Hi Priyanshu,

Thanks for the update, appreciate the work on this 👍

Since this PR includes changes to core files (e.g., package.json and security-related logic), I’m taking a safety-first approach to ensure it doesn’t affect the main runtime or CI behaviour.

Could you please share the test evidence showing everything still works as expected? For example:

CI run link (green checks), and

The test command you ran locally with a brief output/result

Once confirmed and all checks remain green, I’m happy to approve and move this forward. Thanks!

King Hei

Screenshot 2026-01-29 193858 Thanks for checking All the CL checks are green as shown in the screenshot. Please let me know if u need any addiyional details. Thanku

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.

2 participants