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

*: refactor methods to support mocking #63

Merged
merged 2 commits into from
Dec 8, 2024
Merged

Conversation

adityamaru
Copy link

@adityamaru adityamaru commented Dec 8, 2024

Additionally, write some tests to ensure the driver method startBlacksmithBuilder handles all exceptions correctly in both nofallback=true and nofallback=false configurations.


Important

Refactor startBlacksmithBuilder for testability, add tests, and introduce GitHub Actions workflow for CI/CD.

  • Behavior:
    • Refactor startBlacksmithBuilder in main.ts to handle exceptions and support mocking.
    • Add tests for startBlacksmithBuilder in blacksmith-builder.test.ts to verify exception handling with nofallback configurations.
  • Testing:
    • Add blacksmith-builder.test.ts to test startBlacksmithBuilder with different scenarios.
    • Update jest.config.ts to enable coverage collection and adjust test settings.
  • CI/CD:
    • Add build.yml GitHub Actions workflow for build and test automation on push and pull requests.
  • Configuration:
    • Update .eslintrc.json to streamline ESLint configuration for TypeScript.
    • Update package.json to include @types/jest and update ts-jest version.
  • Refactoring:
    • Move reporting functions to reporter.ts and utility functions to utils.ts.
    • Extract getBuilderAddr logic to setup_builder.ts.

This description was created by Ellipsis for f9d1e15. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 14f4f24 in 1 minute and 27 seconds

More details
  • Looked at 2182 lines of code in 10 files
  • Skipped 5 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. jest.config.ts:12
  • Draft comment:
    Consider adding testPathIgnorePatterns to exclude specific test files or directories from being run, if needed.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The Jest configuration file is missing the testPathIgnorePatterns option, which is useful for ignoring specific test files or directories. This can help in excluding certain tests from being run.
2. src/utils.ts:5
  • Draft comment:
    The URL 'http://192.168.127.1:5556' is hardcoded. Consider making this configurable to allow for different environments or endpoints.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_BasnrJy6FUWsk4eo


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

.eslintrc.json Outdated Show resolved Hide resolved
src/reporter.ts Outdated Show resolved Hide resolved
Copy link

ellipsis-dev bot commented Dec 8, 2024

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at help@ellipsis.dev

Additionally, write some tests to ensure the driver method
`startBlacksmithBuilder` handles all exceptions correctly in
both nofallback=true and nofallback=false configurations.
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on c71ad2d in 45 seconds

More details
  • Looked at 2184 lines of code in 10 files
  • Skipped 3 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. .eslintrc.json:9
  • Draft comment:
    Consider adding back the environment settings and parser options that were removed. These settings are important for ensuring the code is linted correctly in the intended environment.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative, suggesting that the removal of settings might be an issue without providing evidence. The rules state to avoid speculative comments and to assume changes are intentional unless there's strong evidence otherwise. The removal might be part of a simplification or refactor.
    I might be overlooking the importance of the removed settings for specific environments or parser options. However, without strong evidence, it's speculative to assume their necessity.
    The critique is valid, but the lack of evidence supporting the necessity of the removed settings means the comment remains speculative.
    Delete the comment as it is speculative and lacks strong evidence that the removal of settings is incorrect.
2. jest.config.ts:20
  • Draft comment:
    Consider adding back the moduleNameMapper configuration if there are any module resolution issues.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The jest.config.ts file is missing a moduleNameMapper configuration that was present before. This could lead to issues if there are any module resolution problems.
3. src/__tests__/blacksmith-builder.test.ts:24
  • Draft comment:
    Ensure that reportBuilderCreationFailed is called with the correct arguments in all test cases where it is expected to be called.
  • Reason this comment was not posted:
    Comment did not seem useful.
4. src/main.ts:77
  • Draft comment:
    Remove the unnecessary log message 'coming to no fallback false' for cleaner code.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The startBlacksmithBuilder function in main.ts logs a message 'coming to no fallback false' which seems unnecessary and could be removed for cleaner code.

Workflow ID: wflow_GKM4NQHWdtqvVB1t


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

.github/workflows/build.yml Show resolved Hide resolved
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 4337e53 in 15 seconds

More details
  • Looked at 333 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. src/main.ts:86
  • Draft comment:
    The error message is constructed twice, which is redundant. Consider constructing it once and using it in both places.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error message in the catch block of startBlacksmithBuilder is being constructed twice, which is redundant. It can be simplified by constructing it once and using it in both places.
2. src/setup_builder.ts:225
  • Draft comment:
    Consider logging the error as an error instead of a warning since the function throws the error afterward.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error message in the catch block of setupStickyDisk is logged as a warning, but it might be more appropriate to log it as an error since the function throws the error afterward.

Workflow ID: wflow_OfDiMaNqiLhH4aOQ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on f9d1e15 in 20 seconds

More details
  • Looked at 333 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. src/main.ts:86
  • Draft comment:
    The error message construction can be simplified to avoid redundancy. Consider constructing the error message once and appending specific details if necessary.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error message in the catch block of the startBlacksmithBuilder function in main.ts is being constructed twice, once with a generic message and once with a specific message for buildkitd errors. This can be simplified by constructing the error message once and then appending the specific message if necessary.
2. src/setup_builder.ts:193
  • Draft comment:
    Ensure consistent error handling by using the same method for throwing errors. Consider using throw new Error(...) for all error messages.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error handling in the startAndConfigureBuildkitd function in setup_builder.ts is not consistent. The error message for the buildkitd socket not being found is thrown as a new Error, but other errors are logged using core.error. This inconsistency can lead to confusion when debugging.
3. src/__tests__/blacksmith-builder.test.ts:56
  • Draft comment:
    Consider using constants for error messages to avoid maintenance issues if the messages change in the main code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test cases in blacksmith-builder.test.ts are using hardcoded strings for error messages. This can lead to maintenance issues if the error messages in the main code change. Consider using constants or a shared resource for error messages.

Workflow ID: wflow_Q2ai2F3zVpNkZs34


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@adityamaru adityamaru merged commit 9f63c68 into master Dec 8, 2024
3 checks passed
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