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(api): Refactor project e2e tests to be more readable #215

Merged
merged 1 commit into from
May 13, 2024

Conversation

rajdip-b
Copy link
Member

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

PR Type

enhancement, tests


Description

  • Simplified the import and configuration of ConfigModule in app.module.ts.
  • Added a default value for JWT_SECRET in auth.module.ts to enhance robustness.
  • Updated project fetching method in project.controller.ts and project.service.ts for clarity.
  • Major enhancements and refactoring in project e2e tests, including better resource management and clearer test cases.
  • Streamlined user creation process in user.service.ts to improve maintainability.

Changes walkthrough 📝

Relevant files
Enhancement
app.module.ts
Simplify ConfigModule Import and Remove Redundant Comment

apps/api/src/app/app.module.ts

  • Removed a comment about the .env file path issue.
  • Simplified the import of ConfigModule.
  • +0/-1     
    auth.module.ts
    Add Fallback for JWT_SECRET in Auth Module                             

    apps/api/src/auth/auth.module.ts

  • Added a fallback default value for JWT_SECRET environment variable.
  • +1/-1     
    project.controller.ts
    Refactor Project Fetch Method in Project Controller           

    apps/api/src/project/controller/project.controller.ts

  • Changed the method used to fetch a project from getProjectByUserAndId
    to getProjectById.
  • +1/-1     
    project.service.ts
    Rename Project Fetch Method in Project Service                     

    apps/api/src/project/service/project.service.ts

  • Renamed getProjectByUserAndId to getProjectById to simplify the method
    name.
  • +1/-1     
    user.service.ts
    Refactor User Creation Process in User Service                     

    apps/api/src/user/service/user.service.ts

  • Refactored user creation to include default workspace creation in a
    single step.
  • Updated logging and email notification to use the new user object.
  • +4/-4     
    Tests
    project.e2e.spec.ts
    Refactor and Enhance Project E2E Tests                                     

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

  • Extensive refactoring and enhancement of project e2e tests.
  • Integration of additional services like UserService, WorkspaceService,
    and ProjectService.
  • Simplified user and project setup for tests.
  • Added and modified several test cases.
  • +82/-106

    💡 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 Description updated to latest commit (62728d1)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and significant changes in logic, especially in the e2e tests and service layers. The changes are well-documented and structured, but the complexity and potential impact on the application's functionality require a thorough review.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The default value for JWT_SECRET in auth.module.ts is set to 'secret', which might not be secure for production environments. Consider using a more secure approach or ensuring this default is only used in development.

    Possible Bug: The refactoring in project.service.ts changes the method name from getProjectByUserAndId to getProjectById but does not update all usages across the application, which could lead to runtime errors if not handled.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Improve the security of the JWT secret fallback value

    Consider using a more secure default value for the JWT secret. Using a simple string like
    'secret' as a fallback is not secure and could potentially expose the application to
    security risks.

    apps/api/src/auth/auth.module.ts [17]

    -secret: process.env.JWT_SECRET ?? 'secret',
    +secret: process.env.JWT_SECRET ?? crypto.randomBytes(64).toString('hex'),
     
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a security risk with using 'secret' as a fallback for JWT secret. Implementing a more secure default using crypto.randomBytes would significantly enhance security.

    9
    Possible bug
    Add error handling for project retrieval to ensure robustness

    Ensure that the method getProjectById is correctly handling permissions and errors, as the
    method name change might reflect a change in functionality or scope.

    apps/api/src/project/controller/project.controller.ts [63]

    -return await this.service.getProjectById(user, projectId)
    +const project = await this.service.getProjectById(user, projectId);
    +if (!project) {
    +  throw new NotFoundException('Project not found');
    +}
    +return project;
     
    Suggestion importance[1-10]: 8

    Why: The suggestion is valid as it addresses error handling which is crucial for robustness, especially after a method name change that could affect functionality.

    8
    Maintainability
    Use a factory for generating user test data to improve maintainability

    Avoid using hardcoded user details directly in test cases. Instead, use a factory or
    fixtures to generate test data, which can help in maintaining and scaling tests more
    effectively.

    apps/api/src/project/project.e2e.spec.ts [72-77]

    -const createUser1 = await userService.createUser({
    -  name: 'John Doe',
    -  email: 'johndoe@keyshade.xyz',
    -  isOnboardingFinished: true,
    -  isActive: true,
    -  isAdmin: false
    -})
    +const createUser1 = await userService.createUser(userFactory.generateUser());
     
    Suggestion importance[1-10]: 6

    Why: Using a factory for user data in tests is a good practice for maintainability and scalability, though it's a relatively minor improvement in the context of the entire project.

    6
    Enhancement
    Rename the method to clearly reflect its functionality including authorization checks

    Consider renaming the method getProjectById to reflect its functionality clearly if it
    includes authorization checks, such as getAuthorizedProjectById.

    apps/api/src/project/service/project.service.ts [289]

    -async getProjectById(user: User, projectId: Project['id']) {
    +async getAuthorizedProjectById(user: User, projectId: Project['id']) {
     
    Suggestion importance[1-10]: 5

    Why: While renaming the method for clarity is beneficial for code readability and maintainability, it's not a critical change and does not impact functionality directly.

    5

    @rajdip-b rajdip-b force-pushed the test/refactor-project-e2e-tests branch from 62728d1 to 4f8c178 Compare May 13, 2024 07:56
    Copy link

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

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

    See analysis details on SonarCloud

    Copy link

    codecov bot commented May 13, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 91.60%. Comparing base (7bb3d21) to head (4f8c178).
    Report is 67 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff              @@
    ##           develop     #215       +/-   ##
    ============================================
    + Coverage    62.20%   91.60%   +29.39%     
    ============================================
      Files           76      110       +34     
      Lines         1503     2465      +962     
      Branches       260      457      +197     
    ============================================
    + Hits           935     2258     +1323     
    + Misses         568      207      -361     
    Flag Coverage Δ
    api-e2e-tests 91.60% <100.00%> (+29.39%) ⬆️

    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 8d0f7c8 into develop May 13, 2024
    7 checks passed
    @rajdip-b rajdip-b deleted the test/refactor-project-e2e-tests branch May 13, 2024 08:00
    @rajdip-b
    Copy link
    Member Author

    🎉 This PR is included in version 1.4.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