Skip to content

Conversation

@PasanMadhuranga
Copy link
Contributor

@PasanMadhuranga PasanMadhuranga commented Oct 13, 2025

Purpose

This PR improves unit test coverage for the @asgardeo/javascript SDK, addressing the gaps identified in issue #154.

Summary of Work Done

  • Fixed and stabilized existing failing tests.

  • Refined and expanded unit tests in:

    • src/api/__tests__/
    • src/errors/__tests__/
  • Added coverage for additional code paths, edge cases, and error handling scenarios to improve reliability and regression detection.

  • Conducted a manual verification round to ensure all tests pass consistently.

Bug Fixes Implemented During Testing

While increasing coverage, a few logic issues were identified and fixed to align with expected SDK behavior.
The following commits address these issues:

Commit Message
113a180 chore(javascript): use numeric ordering for log levels instead of string comparison
539acf3 fix(javascript): handle network and parsing errors gracefully in getUserInfo
446de46 fix(javascript): improve error handling in ExecuteEmbeddedSignInFlow for invalid URLs and network failures
0c7ec47 fix(javascript): improve error handling in executeEmbeddedSignUpFlow for invalid URLs and network failures
f0e4d8b fix(javascript): improve error handling in initializeEmbeddedSignInFlow for invalid URLs and network failures

Observations and Potential Issues

During implementing tests, several inconsistencies and potential bugs were noted that may need team clarification or adjustment:

1. updateMeProfile.ts

Issue:
requestConfig can override the explicitly defined HTTP method.

const requestInit: RequestInit = {
  method: 'PATCH',
  ...requestConfig, // Can override method
  headers: {
    ...requestConfig.headers,
    'Content-Type': 'application/scim+json',
    Accept: 'application/json',
  },
  body: JSON.stringify(data),
};

Suggestion:
Spread requestConfig after defining headers and method to ensure the HTTP method cannot be unintentionally overridden.


2. Multiple API files

Files:
createOrganization.ts, executeEmbeddedSignInFlow.ts, executeEmbeddedSignUpFlow.ts, getBrandingPreference.ts,
getMeOrganizations.ts, getOrganization.ts, getSchemas.ts, getScim2Me.ts, getUserInfo.ts, updateOrganization.ts

Issue:
Content-Type and Accept headers can be overridden by requestConfig.headers.

headers: {
  'Content-Type': 'application/json',
  Accept: 'application/json',
  ...requestConfig.headers, // Overrides defaults
},

Suggestion:
Reverse the merge order if the intention is to enforce standard headers:

headers: {
  ...requestConfig.headers,
  'Content-Type': 'application/json',
  Accept: 'application/json',
},

3. getAllOrganizations.ts, initializeEmbeddedSignInFlow.ts, updateMeProfile.ts

Issue:
Opposite header precedence. Here, requestConfig.headers is overridden by Content-Type and Accept.
This creates inconsistent behavior across API utilities.


4. AsgardeoAPIError.ts

Issue:
toString() renders "undefined" for statusText when not provided.

public override toString(): string {
    const status = this.statusCode ? ` (HTTP ${this.statusCode} - ${this.statusText})` : '';
    return `[${this.name}] (code="${this.code}")${status}\nMessage: ${this.message}`;
  }

Next Steps

  • Continue improving test coverage for other under-tested modules such as utils.
  • Seek confirmation from the team regarding the inconsistencies and potential bugs identified during testing.

Related Issues

Related PRs

  • N/A

Checklist

  • Followed the CONTRIBUTING guidelines.
  • Manual test round performed and verified.
  • Documentation provided. (Add links if there are any)
  • Unit tests provided. (Add links if there are any)

Security checks

@brionmario
Copy link
Member

Hey @PasanMadhuranga,
Great work. I've added few comments.

Also, there's an error in the @asgardeo/javascript package test run. Could you please check this.

@PasanMadhuranga
Copy link
Contributor Author

Hey @PasanMadhuranga, Great work. I've added few comments.

Also, there's an error in the @asgardeo/javascript package test run. Could you please check this.

Thanks a lot @brionmario! ❤️ I really appreciate the feedback. I’ll check the test run error in the @asgardeo/javascript package and get it fixed soon. 😊

@brionmario
Copy link
Member

brionmario commented Oct 24, 2025

@PasanMadhuranga

Shall we fix the test suite errors.
Also, lets add a changeset: https://github.com/asgardeo/javascript/blob/main/CONTRIBUTING.md#creating-a-changeset

  ⎯⎯⎯⎯⎯⎯ Failed Suites 1 ⎯⎯⎯⎯⎯⎯⎯
  
   FAIL  src/utils/__tests__/transformBrandingPreferenceToTheme.test.ts [ src/utils/__tests__/transformBrandingPreferenceToTheme.test.ts ]
  Error: No test suite found in file /home/runner/work/javascript/javascript/packages/javascript/src/utils/__tests__/transformBrandingPreferenceToTheme.test.ts
  ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯
  
  
   Test Files  1 failed | 32 passed (33)
        Tests  307 passed (307)
     Start at  05:30:44
     Duration  5.90s (transform 1.14s, setup 0ms, collect 2.18s, tests 604ms, environment 14ms, prepare 5.58s)
  
   ELIFECYCLE  Test failed. See above for more details.
   ```
   
   

@PasanMadhuranga
Copy link
Contributor Author

Shall we fix the test suite errors. Also, lets add a changeset: https://github.com/asgardeo/javascript/blob/main/CONTRIBUTING.md#creating-a-changeset

Hi @brionmario ! I’ve fixed the test suite errors and also added a changeset.

@asgardeo-github-bot
Copy link

🦋 Changeset detected

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

@brionmario
Copy link
Member

Tests are passing now.
Nice work @PasanMadhuranga.

Merging this...
Screenshot 2025-10-24 at 13 11 18

@brionmario brionmario merged commit 109368a into asgardeo:main Oct 24, 2025
3 of 5 checks passed
@brionmario brionmario added the hacktoberfest-accepted Label required by the Hacktoberfest participating PRs to be listed on the user's profile label Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest-accepted Label required by the Hacktoberfest participating PRs to be listed on the user's profile

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants