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

test: Refactor workspace role e2e tests #248

Merged
merged 2 commits into from
May 28, 2024

Conversation

rajdip-b
Copy link
Member

@rajdip-b rajdip-b commented May 28, 2024

PR Type

Tests, Enhancement


Description

  • Refactored the workspace-role.e2e.spec.ts to use UserService for user creation, improving test setup and teardown with beforeEach and afterEach hooks.
  • Removed direct Prisma calls for user and workspace creation in workspace-role.e2e.spec.ts.
  • Updated test cases in workspace-role.e2e.spec.ts to align with the new setup.
  • Removed cleanUp import and call in afterAll in workspace.e2e.spec.ts.

Changes walkthrough 📝

Relevant files
Tests
workspace-role.e2e.spec.ts
Refactor workspace role e2e tests for improved setup         

apps/api/src/workspace-role/workspace-role.e2e.spec.ts

  • Refactored user creation to use UserService.
  • Added beforeEach and afterEach hooks for test setup and teardown.
  • Removed direct Prisma calls for user and workspace creation.
  • Updated test cases to align with new setup.
  • +107/-183
    workspace.e2e.spec.ts
    Remove cleanup call in workspace e2e tests                             

    apps/api/src/workspace/workspace.e2e.spec.ts

    • Removed cleanUp import and call in afterAll.
    +0/-5     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR primarily involves refactoring and improving test setups, which are generally straightforward to review. The changes are well-documented and focus on enhancing code quality and maintainability.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The removal of direct Prisma calls and replacement with UserService methods need to ensure that all user properties and relationships are correctly handled to avoid breaking changes in user management.

    Redundancy Issue: The WorkspaceRoleModule is imported twice in the beforeAll setup which might be a mistake or oversight.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Enhance cleanup logic to include deletion of roles to prevent state leakage between tests

    Implement cleanup logic in the afterEach block to remove any created roles, ensuring that
    each test runs in a clean state. This is crucial for avoiding state leakage between tests
    which can lead to flaky tests.

    apps/api/src/workspace-role/workspace-role.e2e.spec.ts [178-181]

     afterEach(async () => {
       await prisma.$transaction([
         prisma.user.deleteMany(),
    -    prisma.workspace.deleteMany()
    +    prisma.workspace.deleteMany(),
    +    prisma.workspaceRole.deleteMany()  # Ensure roles are also cleaned up
       ])
     })
     
    Suggestion importance[1-10]: 10

    Why: Adding cleanup logic for roles in the afterEach block is crucial for test isolation and reliability. It prevents state leakage between tests, ensuring each test runs in a clean environment.

    10
    Possible issue
    Remove duplicate module import to clean up the code

    Remove the duplicate import of WorkspaceRoleModule to avoid potential issues and
    confusion. Having a module imported multiple times in the same array does not serve any
    purpose and could lead to misleading interpretations of the module's usage in the
    application.

    apps/api/src/workspace-role/workspace-role.e2e.spec.ts [50-52]

     imports: [
       AppModule,
       WorkspaceRoleModule,
       EventModule,
    -  WorkspaceRoleModule,
       UserModule
     ]
     
    Suggestion importance[1-10]: 9

    Why: Removing the duplicate import of WorkspaceRoleModule is a good practice to avoid confusion and potential issues. It cleans up the code and ensures clarity in module usage.

    9
    Enhancement
    Use transactions to ensure data consistency during user creation and role assignments

    Consider handling exceptions for user creation and role assignments within a transaction
    to ensure data consistency and rollback in case of errors. This approach helps maintain
    database integrity by ensuring that all operations either complete successfully together
    or fail together.

    apps/api/src/workspace-role/workspace-role.e2e.spec.ts [73-127]

    -const createAlice = await userService.createUser({
    -  email: 'alice@keyshade.xyz',
    -  name: 'Alice',
    -  isActive: true,
    -  isAdmin: false,
    -  isOnboardingFinished: true
    -})
    -...
    -await workspaceRoleService.createWorkspaceRole(alice, workspaceAlice.id, {
    -  name: 'Member',
    -  description: 'Member Role',
    -  colorCode: '#0000FF',
    -  authorities: [Authority.READ_WORKSPACE_ROLE, Authority.READ_WORKSPACE]
    +await prisma.$transaction(async () => {
    +  const createAlice = await userService.createUser({
    +    email: 'alice@keyshade.xyz',
    +    name: 'Alice',
    +    isActive: true,
    +    isAdmin: false,
    +    isOnboardingFinished: true
    +  })
    +  ...
    +  await workspaceRoleService.createWorkspaceRole(alice, workspaceAlice.id, {
    +    name: 'Member',
    +    description: 'Member Role',
    +    colorCode: '#0000FF',
    +    authorities: [Authority.READ_WORKSPACE_ROLE, Authority.READ_WORKSPACE]
    +  })
     })
     
    Suggestion importance[1-10]: 8

    Why: Using transactions for user creation and role assignments enhances data consistency and integrity. This is a significant improvement for maintaining database reliability.

    8
    Maintainability
    Refactor user creation into a helper function for better code organization and maintenance

    Refactor the repeated user creation logic into a helper function to improve code
    readability and reusability. This change will make the test setup more concise and easier
    to maintain.

    apps/api/src/workspace-role/workspace-role.e2e.spec.ts [73-95]

    -const createAlice = await userService.createUser({
    -  email: 'alice@keyshade.xyz',
    -  name: 'Alice',
    -  isActive: true,
    -  isAdmin: false,
    -  isOnboardingFinished: true
    -})
    -const createBob = await userService.createUser({
    -  email: 'bob@keyshade.xyz',
    -  name: 'Bob',
    -  isActive: true,
    -  isAdmin: false,
    -  isOnboardingFinished: true
    -})
    -const createCharlie = await userService.createUser({
    -  email: 'charlie@keyshade.xyz',
    -  name: 'Charlie',
    -  isActive: true,
    -  isAdmin: false,
    -  isOnboardingFinished: true
    -})
    +async function createUser(email, name) {
    +  return await userService.createUser({
    +    email: email,
    +    name: name,
    +    isActive: true,
    +    isAdmin: false,
    +    isOnboardingFinished: true
    +  });
    +}
    +const createAlice = await createUser('alice@keyshade.xyz', 'Alice');
    +const createBob = await createUser('bob@keyshade.xyz', 'Bob');
    +const createCharlie = await createUser('charlie@keyshade.xyz', 'Charlie');
     
    Suggestion importance[1-10]: 7

    Why: Refactoring user creation into a helper function improves code readability and maintainability. While beneficial, it is a minor enhancement compared to the other suggestions.

    7

    Copy link

    codecov bot commented May 28, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 90.61%. Comparing base (ce50743) to head (64983e0).
    Report is 15 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #248      +/-   ##
    ===========================================
    - Coverage    91.71%   90.61%   -1.11%     
    ===========================================
      Files          111      113       +2     
      Lines         2510     2737     +227     
      Branches       469      524      +55     
    ===========================================
    + Hits          2302     2480     +178     
    - Misses         208      257      +49     
    Flag Coverage Δ
    api-e2e-tests 90.61% <ø> (-1.11%) ⬇️

    Flags with carried forward coverage won't be shown. Click here to find out more.

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

    @rajdip-b rajdip-b merged commit 665ff96 into develop May 28, 2024
    4 checks passed
    @rajdip-b rajdip-b deleted the test/refactor-workspace-role-e2e-test branch May 28, 2024 17:15
    Copy link

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    2.0% Duplication on New Code

    See analysis details on SonarCloud

    HarshPatel5940 pushed a commit to HarshPatel5940/keyshade that referenced this pull request Jun 1, 2024
    @rajdip-b
    Copy link
    Member Author

    🎉 This PR is included in version 2.0.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant