Node18/conformance test

[thiyaguk09]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Description

Please provide a detailed description for the change.
As much as possible, please try to keep changes separate by purpose. For example, try not to make a one-line bug fix in a feature request, or add an irrelevant README change to a bug fix.

Impact

What's the impact of this change?

Testing

Have you added unit and integration tests if necessary?
Were any tests changed? Are any breaking changes necessary?

Additional Information

Any additional details that we should be aware of?

Checklist

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease
• Appropriate docs were updated
• Appropriate comments were added, particularly in complex areas or places that require background
• No new warnings or issues will be generated from this change

Fixes #issue_number_goes_here 🦕

[gemini-code-assist]

Summary of Changes

Hello @thiyaguk09, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the conformance test suite by shifting many test methods to directly interact with the StorageTransport layer. This change allows for more precise control over HTTP requests, which is crucial for thoroughly testing retry mechanisms and other low-level API behaviors. Additionally, it enhances the robustness of the default retry logic and improves error handling within the transport layer.

Highlights

• Conformance Test Refactoring: Many conformance test methods for bucket, file, HMAC key, IAM, notification, and storage operations have been refactored to directly use StorageTransport.makeRequest instead of the higher-level Storage client methods. This provides more granular control over API requests for testing purposes.
• Retry Logic Enhancement: The default retryable error function (RETRYABLE_ERR_FN_DEFAULT) has been improved to include additional connection-related error codes (e.g., etimedout, econnrefused) and to check for specific retryable reasons within nested error structures (e.g., ratelimitexceeded).
• Request Interception for Tests: A Proxy is now used for StorageTransport within conformance tests to intercept makeRequest calls and automatically inject the x-retry-test-id header, streamlining the testing of retry scenarios.
• Dependency Update: The html-entities dependency has been removed from package.json.
• StorageTransport Improvements: The StorageTransport.makeRequest method now explicitly passes queryParameters to gaxios and sets validateStatus to only consider 2xx responses as successful. It also includes enhanced error logging for GaxiosError instances.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors the conformance tests to interact with the transport layer directly, which is a solid approach for testing retry logic. The changes also include improvements to the default retry function in the main library. The overall direction is good, but I've identified a few issues, primarily concerning error handling, type safety, and a bug in header manipulation within the test suite. Addressing these will improve the robustness and reliability of the tests.

[thiyaguk09]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the conformance tests to use the transport layer directly, allowing for more granular testing of retry logic. The changes are extensive and mostly involve rewriting library methods to make direct REST API calls. I've found a couple of critical issues in the new test setup logic that could lead to runtime errors or incorrect behavior. Additionally, there are a few instances of commented-out code that should be removed for cleanliness. The improvements to the retryable error detection function in src/storage.ts are well-done and increase robustness.

[thiyaguk09]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces significant refactoring to the conformance tests, primarily to align with Node.js 18 and to test retry logic at the transport layer. The changes involve bypassing high-level SDK methods in favor of direct calls to storageTransport.makeRequest, implementing a complex custom retry strategy, and updating various test helper functions. My review focuses on improving type safety, removing leftover debugging code, and enhancing code clarity in these new implementations.

[thiyaguk09]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the conformance tests to use a lower-level transport layer for making requests, which provides better control for retry tests. The changes also include a more sophisticated and specific retry logic in the StorageTransport. The overall direction is good, but I've found a few areas for improvement: some test helper functions are still using the old SDK methods, and there's a potential bug in header handling within the transport layer. Additionally, the new retry logic could be refactored for better readability.

[thiyaguk09]

/gemini review

[thiyaguk09]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant refactoring of the conformance tests to directly target the transport layer, bypassing the high-level client methods. This is a great step for ensuring low-level correctness. The PR also includes a crucial improvement to the retry logic by making it idempotency-aware, which greatly enhances the safety and correctness of request retries. My review focuses on the new test implementations and the updated retry mechanism. I've identified a potential unhandled exception in the test setup and suggested improvements for type safety in the new test methods.

[gemini-code-assist]

The check for bucket existence at line 124 will throw an unhandled exception if the bucket does not exist, because makeRequest rejects on non-2xx status codes like 404. This will cause the test to fail instead of proceeding to create the bucket. You should wrap this call in a try...catch block to handle the 404 case gracefully, similar to how it's done in the bucketExists function in this same file.

  try {
    await options.storageTransport.makeRequest(existsReq);
    bucketExists = true;
  } catch (e) {
    const err = e as GaxiosError;
    if (err.response?.status !== 404) {
      throw e;
    }
  }

[gemini-code-assist]

The use of as any to access response properties like items and nextPageToken bypasses TypeScript's type safety. This pattern is repeated throughout the file. It would be more robust to define simple interfaces for the expected API responses and use generics with makeRequest.

For example, for this object listing operation, you could do:

interface ObjectList {
  items?: { name: string }[];
  nextPageToken?: string;
}

// ... in the loop ...
const listResult = await options.storageTransport.makeRequest<ObjectList>(listReq);
const objects = listResult?.items || [];
// ... later ...
pageToken = listResult?.nextPageToken;

This would improve code clarity, maintainability, and help catch potential issues at compile time.