feat: add protobuf #717
feat: add protobuf #717
Conversation
WalkthroughThe changes in this pull request introduce a new entry for the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
非常欢迎你参与贡献，请确保你的新功能或者缺陷修复都包含相对于的单元测试用例，方便我们快速理解你的意图。更多贡献者说明请参考 https://github.com/cnpm/cnpmcore/blob/main/DEVELOPER.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
test/common/adapter/binary/GithubBinary.test.ts (1)
118-118: Consider removing the console.log statement.The
console.logstatement on this line is likely used for debugging purposes. It's generally a good practice to remove such statements from test code before committing, as they can clutter the test output.- console.log(JSON.stringify(result.items, null, 2));config/binaries.ts (1)
141-141: Consider removing quotes from the 'protobuf' property name.The static analysis tool has flagged an issue with unnecessarily quoted property 'protobuf'. In JavaScript and TypeScript, it's generally recommended to avoid quoting property names unless they contain special characters or are invalid identifiers.
Consider applying this change:
- 'protobuf': { + protobuf: {This change will make the code more consistent with modern JavaScript/TypeScript style guidelines and remove the unnecessary quotes.
🧰 Tools
🪛 GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found🪛 GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found🪛 GitHub Check: test-mysql57-fs-nfs (18, ubuntu-latest)
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- config/binaries.ts (1 hunks)
- test/common/adapter/binary/GithubBinary.test.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
config/binaries.ts
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found
🪛 GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
config/binaries.ts
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found
🪛 GitHub Check: test-mysql57-fs-nfs (18, ubuntu-latest)
config/binaries.ts
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found
🔇 Additional comments (1)
config/binaries.ts (1)
141-147: LGTM! New 'protobuf' entry looks good.The new entry for 'protobuf' is well-structured and consistent with other entries in the
binariesobject. It includes all the necessary properties:
category: Correctly set to 'protobuf'description: Provides a clear and concise description of Protocol Bufferstype: Correctly set toBinaryType.GitHubrepo: Points to the correct GitHub repository 'protocolbuffers/protobuf'distUrl: Correctly set to the releases page of the repositoryThe addition of this entry will enable the system to handle Protocol Buffers binary distributions.
🧰 Tools
🪛 GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found🪛 GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found🪛 GitHub Check: test-mysql57-fs-nfs (18, ubuntu-latest)
[failure] 141-141:
Unnecessarily quoted property 'protobuf' found
| let matchFile1 = false; | ||
| for (const item of result.items) { | ||
| assert(item.isDir === false); | ||
| if (item.name === 'protoc-28.2-linux-aarch_64.zip') { | ||
| assert(item.date === '2024-09-18T21:02:40Z'); | ||
| assert(item.size === 3218760); | ||
| assert.equal(item.url, 'https://github.com/protocolbuffers/protobuf/releases/download/v28.2/protoc-28.2-linux-aarch_64.zip'); | ||
| matchFile1 = true; | ||
| } | ||
| } | ||
| assert(matchFile1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage by checking multiple files.
The current test only verifies the presence and properties of a single file (protoc-28.2-linux-aarch_64.zip). To improve test coverage and ensure the fetch method works correctly for various file types, consider adding checks for additional files with different architectures or operating systems.
Here's an example of how you could expand the test:
const expectedFiles = [
{
name: 'protoc-28.2-linux-aarch_64.zip',
date: '2024-09-18T21:02:40Z',
size: 3218760,
url: 'https://github.com/protocolbuffers/protobuf/releases/download/v28.2/protoc-28.2-linux-aarch_64.zip'
},
{
name: 'protoc-28.2-linux-x86_64.zip',
date: '2024-09-18T21:02:40Z',
size: 3456789,
url: 'https://github.com/protocolbuffers/protobuf/releases/download/v28.2/protoc-28.2-linux-x86_64.zip'
},
// Add more expected files here
];
for (const expectedFile of expectedFiles) {
let fileFound = false;
for (const item of result.items) {
if (item.name === expectedFile.name) {
assert(item.date === expectedFile.date);
assert(item.size === expectedFile.size);
assert.equal(item.url, expectedFile.url);
fileFound = true;
break;
}
}
assert(fileFound, `File ${expectedFile.name} not found in the result`);
}This approach allows you to easily add more files to check without duplicating assertion code.
akitaSummer
force-pushed
the
feat/protobuf
branch
from
1e09aa6 to
b06e255
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #717 +/- ##
=======================================
Coverage 96.46% 96.46%
=======================================
Files 188 188
Lines 18796 18803 +7
Branches 2463 2463
=======================================
+ Hits 18132 18139 +7
Misses 664 664 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- config/binaries.ts (1 hunks)
- test/common/adapter/binary/GithubBinary.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/binaries.ts
🧰 Additional context used
| it('should fetch protobuf', async () => { | ||
| const response = await TestUtil.readJSONFile(TestUtil.getFixtures('protobuf-releases.json')); | ||
| app.mockHttpclient(/https:\/\/api\.github\.com\/repos\/protocolbuffers\/protobuf\/releases/, 'GET', { | ||
| data: response, | ||
| status: 200, | ||
| }); | ||
| let result = await binary.fetch('/', 'protobuf'); | ||
| assert(result); | ||
| assert(result.items.length > 0); | ||
| let matchDir = false; | ||
| for (const item of result.items) { | ||
| assert(item.isDir === true); | ||
| if (item.name === 'v28.2/') { | ||
| matchDir = true; | ||
| } | ||
| } | ||
| assert(matchDir); | ||
|
|
||
| result = await binary.fetch('/v28.2/', 'protobuf'); | ||
| assert(result?.items.every(item => !/{.*}/.test(item.url))); | ||
|
|
||
| result = await binary.fetch('/v28.2/', 'protobuf'); | ||
| assert(result); | ||
| assert(result.items.length > 0); | ||
| console.log(JSON.stringify(result.items, null, 2)); | ||
| let matchFile1 = false; | ||
| for (const item of result.items) { | ||
| assert(item.isDir === false); | ||
| if (item.name === 'protoc-28.2-linux-aarch_64.zip') { | ||
| assert(item.date === '2024-09-18T21:02:40Z'); | ||
| assert(item.size === 3218760); | ||
| assert.equal(item.url, 'https://github.com/protocolbuffers/protobuf/releases/download/v28.2/protoc-28.2-linux-aarch_64.zip'); | ||
| matchFile1 = true; | ||
| } | ||
| } | ||
| assert(matchFile1); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring and enhancing the test case.
While the new test case for fetching protobuf is functional, there are several areas for improvement:
-
Code duplication: The structure is very similar to the existing "should fetch skia-canvas" test. Consider refactoring common logic into a helper function to reduce duplication and improve maintainability.
-
Test coverage: The test only checks for a single file. Enhance coverage by verifying multiple files with different architectures or operating systems.
-
Debug logging: Remove the
console.logstatement on line 118 before finalizing the code. -
Error handling: Add test cases for potential error scenarios, such as API failures or unexpected response formats.
Here's a suggested refactoring to address these points:
function testGithubBinaryFetch(repoName: string, fixtureName: string, expectedDir: string, expectedFiles: Array<{name: string, date: string, size: number, url: string}>) {
it(`should fetch ${repoName}`, async () => {
const response = await TestUtil.readJSONFile(TestUtil.getFixtures(fixtureName));
app.mockHttpclient(new RegExp(`https://api.github.com/repos/.+/${repoName}/releases`), 'GET', {
data: response,
status: 200,
});
let result = await binary.fetch('/', repoName);
assert(result && result.items.length > 0);
assert(result.items.some(item => item.name === expectedDir && item.isDir));
result = await binary.fetch(`/${expectedDir}`, repoName);
assert(result?.items.every(item => !/{.*}/.test(item.url)));
result = await binary.fetch(`/${expectedDir}`, repoName);
assert(result && result.items.length > 0);
for (const expectedFile of expectedFiles) {
const matchingFile = result.items.find(item => item.name === expectedFile.name);
assert(matchingFile, `File ${expectedFile.name} not found`);
assert(matchingFile.date === expectedFile.date);
assert(matchingFile.size === expectedFile.size);
assert.equal(matchingFile.url, expectedFile.url);
}
});
}
// Usage
testGithubBinaryFetch(
'protobuf',
'protobuf-releases.json',
'v28.2/',
[
{
name: 'protoc-28.2-linux-aarch_64.zip',
date: '2024-09-18T21:02:40Z',
size: 3218760,
url: 'https://github.com/protocolbuffers/protobuf/releases/download/v28.2/protoc-28.2-linux-aarch_64.zip'
},
// Add more expected files here
]
);
// Add error case test
it('should handle API failure', async () => {
app.mockHttpclient(/https:\/\/api\.github\.com\/repos\/protocolbuffers\/protobuf\/releases/, 'GET', {
status: 500,
});
await assert.rejects(async () => {
await binary.fetch('/', 'protobuf');
}, /API request failed/);
});This refactoring addresses the code duplication, allows for easier addition of more test cases, and includes an example of error case testing.
akitaSummer
force-pushed
the
feat/protobuf
branch
from
b06e255 to
af52c66
Compare
[skip ci] ## [3.64.0](v3.63.1...v3.64.0) (2024-10-18) ### Features * mirror protobuf binary ([#717](#717)) ([d6f0e1d](d6f0e1d))
Summary by CodeRabbit
New Features
Tests