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

feat: add protobuf #717

Merged
merged 1 commit into from
Oct 18, 2024
+14,237 −0
Merged

feat: add protobuf #717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions config/binaries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,13 @@ const binaries = {
repo: 'samizdatco/skia-canvas',
distUrl: 'https://github.com/samizdatco/skia-canvas/releases',
},
protobuf: {
category: 'protobuf',
description: 'Protocol Buffers - Google\'s data interchange format',
type: BinaryType.GitHub,
repo: 'protocolbuffers/protobuf',
distUrl: 'https://github.com/protocolbuffers/protobuf/releases',
},
wrtc: {
category: 'wrtc',
description: 'node-webrtc is a Node.js Native Addon that provides bindings to WebRTC M87.',
Expand Down
38 changes: 38 additions & 0 deletions test/common/adapter/binary/GithubBinary.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,43 @@ describe('test/common/adapter/binary/GithubBinary.test.ts', () => {
assert(matchFile2);
assert(matchFile3);
});

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);
Comment on lines +119 to +129
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

});
Comment on lines +94 to +130
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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:

  1. 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.

  2. Test coverage: The test only checks for a single file. Enhance coverage by verifying multiple files with different architectures or operating systems.

  3. Debug logging: Remove the console.log statement on line 118 before finalizing the code.

  4. 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.

});
});
Loading
Loading