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.

Sign up for GitHub

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

Jump to bottom

feat: Add cross-platform sleep utility for e2e preparation #523

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

feat: Add cross-platform sleep utility for e2e preparation #523

wants to merge 4 commits into from

Conversation

Allan2000-Git
Copy link
Contributor

@Allan2000-Git Allan2000-Git commented Nov 8, 2024 •
edited by codiumai-pr-agent-free bot
Loading

User description

Description

Implement cross-platform sleep functionality for e2e tests

Fixes #506

Dependencies

Installed ts-node package to transpile typescript to javascript

Future Improvements

N/A

Mentions

@rajdip-b

Screenshots of relevant screens

N/A

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement, dependencies

Description

  • Implemented a cross-platform sleep utility to replace the sleep command with OS-specific commands, enhancing compatibility across different operating systems.
  • Updated the e2e:prepare script in package.json to utilize the new cross-platform sleep utility.
  • Added ts-node and tsx as development dependencies to support TypeScript execution.
  • Set the package type to module to align with modern JavaScript standards.

Changes walkthrough 📝

Relevant files
Enhancement
cross-platform-sleep.ts
Implement cross-platform sleep utility for OS-specific commands

apps/api/src/common/cross-platform-sleep.ts

  • Implemented a cross-platform sleep utility using execSync.
  • Added OS-specific commands for Windows, macOS, and Linux.
  • Included error handling for unsupported operating systems.
    		•  +22/-0   
    Dependencies
    package.json
    Update package.json for cross-platform sleep and dependencies

    apps/api/package.json

  • Added ts-node and tsx as development dependencies.
  • Modified e2e:prepare script to use the new cross-platform sleep
    utility.
  • Set package type to module.
    		•  +6/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    506 - Fully compliant

    Fully compliant requirements:

    • Implemented OS-specific sleep commands for Windows (powershell), Linux and macOS
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Command Injection:
    The seconds variable is used directly in command string construction. While currently hard-coded, if this becomes configurable in the future, it could be vulnerable to command injection if not properly sanitized.

    ⚡ Recommended focus areas for review

    Hard-coded Value
    The sleep duration is hard-coded to 3 seconds. Consider making this configurable through an environment variable or command line argument.

    Error Handling
    The error message from execSync is logged but could contain sensitive information. Consider sanitizing or limiting the error output.

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Nov 8, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation to ensure sleep duration is positive to prevent runtime errors

    Add input validation to ensure the sleep duration is a positive number to prevent
    potential issues with invalid sleep times.

    apps/api/src/common/cross-platform-sleep.ts [4-5]

     const seconds = 3;
+if (seconds <= 0) {
+  console.error('Sleep duration must be a positive number');
+  process.exit(1);
+}
 const os = platform();
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Input validation is crucial for preventing runtime errors and ensuring the sleep utility functions correctly. This is especially important as the value could be provided via command line arguments.

    		8
    Enhancement
    Make sleep duration configurable via command line arguments instead of hardcoding the value

    Make the sleep duration configurable by accepting it as a command line argument
    instead of hardcoding it to 3 seconds. This allows more flexibility when using the
    utility.

    apps/api/src/common/cross-platform-sleep.ts [4-5]

    -const seconds = 3;
+const seconds = parseInt(process.argv[2]) || 3;
 const os = platform();
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Making the sleep duration configurable improves the utility's flexibility and reusability, especially since it's being used in build scripts where different wait times might be needed.

    		7
    Best practice
    Maintain consistent string formatting style across error messages

    Use template literals consistently for error messages to maintain code style
    uniformity.

    apps/api/src/common/cross-platform-sleep.ts [13-14]

    -console.error('Unsupported operating system');
+console.error(`Unsupported operating system: ${os}`);
 process.exit(1);
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While consistent string formatting improves code style uniformity, this suggestion has minimal impact on functionality. Adding the OS to the error message provides slightly better debugging information.

    		3

    💡 Need additional feedback ? start a PR chat

    @Allan2000-Git
    Copy link
    Contributor Author

    Allan2000-Git commented Nov 8, 2024 •
    edited
    Loading

    @rajdip-b It's working but giving below error. It should not actually because other files are also using import

    import { execSync } from 'child_process' ^^^^^^ SyntaxError: Cannot use import statement outside a module

    Possible solution:
    "module": "ESNext", "moduleResolution": "Node"

    rajdip-b
    rajdip-b requested changes Nov 8, 2024
    View reviewed changes
    apps/api/src/common/cross-platform-sleep.ts Outdated Show resolved Hide resolved
    apps/api/src/common/cross-platform-sleep.ts Outdated Show resolved Hide resolved
    @rajdip-b
    Copy link
    Member

    rajdip-b commented Nov 8, 2024

    Possible solution: "module": "ESNext", "moduleResolution": "Node"

    may be you can try doing this

    @Allan2000-Git
    Copy link
    Contributor Author

    This is causing a lot of troubles

    1. It doesn't detect type as module
    2. Even if it detects, lint file gives error saying that type commonjs is required
    3. If i change to commonjs, other files gets affected

    I do not have a solution.

    @rajdip-b
    Copy link
    Member

    Ah man this is bad.

    @Allan2000-Git Allan2000-Git force-pushed the feat/os-specific-sleep-command branch from ae0f1ed to 3ba51a2 Compare December 5, 2024 18:48
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @rajdip-b rajdip-b rajdip-b requested changes

    Requested changes must be addressed to merge this pull request.

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    API: Replace sleep command with OS-specific command
    2 participants
    @Allan2000-Git @rajdip-b