feat: add source registry name in manifest #448
Conversation
elrrrrrrr
commented
Apr 24, 2023
•
elrrrrrrr
commented
- 🧶 Add related types for PackageManifestType and adjust relevant unit tests.
- 🤖 Update the workflow trigger.
- ♻️ No compensation will be made for the _source_registry_name field in the existing packageVersion.
- 🧶 新增 PackageManifestType 相关类型，并调整相关单测
- 🤖 调整 workflow 触发时机，不限制 target 分支
- ♻️ 存量 packageVersion 内 _source_registry_name 不做补偿
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report
@@ Coverage Diff @@
## master #448 +/- ##
==========================================
- Coverage 97.41% 97.27% -0.14%
==========================================
Files 159 159
Lines 14480 14652 +172
Branches 1871 1871
==========================================
+ Hits 14105 14253 +148
- Misses 375 399 +24
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| return registry; | ||
|
|
||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
这段代码是在RegistryManagerService类中添加了一个async函数：ensureDefaultRegistry，作用是确保默认Registry的存在。主要功能如下：
-
在registryRepository中查找是否有已存在的默认Registry，如果有则直接返回。
-
如果没有已存在的默认Registry，直接从config.cnpmcore配置文件中获取一些参数，并通过createRegistry方法生成一个新的Registry。
-
返回此新注册表。
这个函数看起来逻辑较为清晰，但是缺少对异常情况的处理。例如，如果connect到数据库失败会如何处理。另外，也许可以考虑使用TypeScript中的Optional chaining（可选链操作符）避免某些属性值为空导致的运行时错误。
| @@ -128,7 +128,6 @@ describe('test/cli/npm/install.test.ts', () => { | |||
| cwd: demoDir, | |||
| }) | |||
| .debug() | |||
| .expect('stdout', /added 1 package/) | |||
| .expect('code', 0) | |||
| .end(); | |||
|
|
|||
There was a problem hiding this comment.
该代码补丁中，移除了一行断言 expect('stdout', /added 1 package/)。
没有明显的风险或错误。但如果有必要检查目标是否是确切的示例。提出建议：在测试通过后添加更多具体的功能测试。
| const data = res.body as PackageManifestType; | ||
| assert(Object.values(data.versions).every(v => v!._source_registry_name === 'self')); | ||
| }); | ||
|
|
||
| it('should not throw error if no versions', async () => { | ||
| const bugVersion = new BugVersion({ | ||
| 'testmodule-show-package': { |
There was a problem hiding this comment.
此代码补丁仅更改了import语句和添加了两个测试用例。其中一个测试用例检查是否正确显示源注册表名称，另一个测试用例检查是否在以缩写形式请求时正确显示源注册表名称。
在代码方面没有明显的错误风险，但是可以考虑在测试中增加边界测试或者对代码进行一些优化，如对异步操作进行真实的Mock等等。
elrrrrrrr
force-pushed
the
add-registry-name
branch
from
d91304f
to
ea63ef5
Compare
| return registry; | ||
|
|
||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
这段代码为 RegistryManagerService 类添加了一个新的方法 ensureDefaultRegistry()，用于创建默认的仓库。
在这个方法中，首先查询是否已存在名为 'default' 的仓库，如果存在则直接返回；否则从配置文件中生成一个新的仓库，并将其保存到数据库中。
其中，类型默认是 npm，如果配置文件指定为 cnpmcore，则类型为 cnpmcore。此外，还需要根据配置文件中的信息设置一些其他的参数，例如 userPrefix、host 和 changeStream。
需要注意的是，在这个方法中并没有对输入参数进行任何检查和验证，因此可能存在一些潜在的风险。建议添加一些参数验证和错误处理机制，增强代码的健壮性。
| @@ -128,7 +128,6 @@ describe('test/cli/npm/install.test.ts', () => { | |||
| cwd: demoDir, | |||
| }) | |||
| .debug() | |||
| .expect('stdout', /added 1 package/) | |||
| .expect('code', 0) | |||
| .end(); | |||
|
|
|||
There was a problem hiding this comment.
这段代码补丁中对测试用例进行了更改，移除了输出中包含“added 1 package”的期望，并保留了测试用例正常退出的期望。从代码片段来看，未发现任何潜在的风险和改进建议。
| const data = res.body as PackageManifestType; | ||
| assert(Object.values(data.versions).every(v => v!._source_registry_name === 'self')); | ||
| }); | ||
|
|
||
| it('should not throw error if no versions', async () => { | ||
| const bugVersion = new BugVersion({ | ||
| 'testmodule-show-package': { |
There was a problem hiding this comment.
这段代码应该是对 Egg.js 项目中的一个 PackageManagerService 进行测试的代码补丁。以下是几个改进建议和存在的潜在风险：
- import assert from 'assert'; 应该写成 const assert = require('assert'); （如果未开启 ES module 的话）或者使用 import * as assert from 'assert';
- 基本上，无论何时使用 mock 来测试某个对象时，都需要在 afterEach() 中还原它，以避免其影响其他测试用例，这里没有看到还原操作。
- 在新引入的 PackageManifestType 类型中，version 字段被标记为可选类型，但是当 v 参数传递给 Object.values 后，返回的值可能不存在，导致 _.source_registry_name 无法访问。可以加入 _.version! ?? '' 来确保 _.version 存在。
- 如果出现错误，使用 assert.rejects 或 await expect(xxx).rejects.toThrow() 语句可以比较方便地断言代码是否符合预期，并输出更有意义的错误信息。
- 特定请求的 Content-Type 可以改为变量来表示，以使其更易于管理。
除了上述问题外，这个代码补丁看起来很不错，测试用例的数量也相对较多。
| .set('accept', 'application/vnd.npm.install-v1+json') | ||
| .expect(200); | ||
| assert(res.body._source_registry_name === 'self'); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
这段代码是一个测试用例，在 npm 包管理器框架下的一些功能进行了单元测试。
第一个测试用例测试一个不存在的包，是否可以正确跳转到 NPM 官方网站。该测试没有明显的错误和需要改进点。
第二个测试用例检查版本清单中的 _source_registry_name 是否正确设置为 self。测试前会创建一个版本号为 1.0.0 的 npm 包“@cnpm/foo”进行测试。该测试也没有明显的错误和需要改进点。
elrrrrrrr
force-pushed
the
add-registry-name
branch
from
ea63ef5
to
b0f9ba4
Compare
| return registry; | ||
|
|
||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
这段代码看起来是在一个叫做 RegistryManagerService 的类里增加了一个 async 函数 ensureDefaultRegistry，这个函数返回一个 Promise 对象，它的返回值是一个叫做 Registry 的对象。该函数先通过调用 registryRepository 的 findRegistry 方法查找是否已经存在一个名为 default 的仓库，如果存在，则直接返回找到的仓库。如果不存在，则从配置文件中读取信息，创建一个新的仓库并返回它。
从代码上看不出来有什么明显的 bug 风险。不过如果这个仓库重要程度很高，可以考虑对 createRegistry 方法参数的校验和输入的限制。
关于改进建议，可以添加一些参数来提高 ensureDefaultRegistry 函数的灵活性，比如允许用户传入一个自定义的 RegistryType 和 host。此外，还可以增加一些注释或者日志来方便维护和调试。
| @@ -128,7 +128,6 @@ describe('test/cli/npm/install.test.ts', () => { | |||
| cwd: demoDir, | |||
| }) | |||
| .debug() | |||
| .expect('stdout', /added 1 package/) | |||
| .expect('code', 0) | |||
| .end(); | |||
|
|
|||
There was a problem hiding this comment.
这段代码是一个测试用例，它使用的是Jest框架。根据这段代码，以下是我的一些观察和建议：
- 该测试案例似乎执行了一些安装操作，因此我希望能够看到在执行安装过程中输出的一些详细信息，例如安装的软件包名称。考虑向
expect()函数添加更多的匹配器/matchers。 - 删除
.expect('stdout', /added 1 package/)行并不会影响测试结果，因为同时检查运行状态(.expect('code', 0))也已足够确认是否正确执行了正确的操作。 - 将调试(
.debug())和链式(.end())方法移除，在错误发生时可能更容易得到有用的调试信息。 - 检查测试环境，确保在测试前剔除任何缓存、临时文件等，否则这些因素可能对测试结果产生影响。
需要注意的是，以上意见只基于给出的代码片段而提出，并不能确诊整个程序。
| const data = res.body as PackageManifestType; | ||
| assert(Object.values(data.versions).every(v => v!._source_registry_name === 'self')); | ||
| }); | ||
|
|
||
| it('should not throw error if no versions', async () => { | ||
| const bugVersion = new BugVersion({ | ||
| 'testmodule-show-package': { |
There was a problem hiding this comment.
该代码补丁有两个测试用例，主要添加了"_source_registry_name"字段的显示。其中第一个测试用例验证所有版本是否显示"_source_registry_name"为"self"。第二个测试用例验证通过"application/vnd.npm.install-v1+json"头部接受缩写格式的包时是否能正常显示"_source_registry_name"。
在第一个测试用例中可以看出其并没有进行任何测试，相当于一个空的测试用例。在第二个测试用例中，虽然测试了一定功能但是在对缩写格式的处理上可能存在风险。建议在code review时添加更加详细和全面的测试，并对与缩写格式相关的处理进行优化，并注释清晰易懂。
| .set('accept', 'application/vnd.npm.install-v1+json') | ||
| .expect(200); | ||
| assert(res.body._source_registry_name === 'self'); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
这份代码补丁的作用是给一个 Node.js 应用程序添加新功能。下面是我的一些建议：
- 在测试方法中，应该使用测试框架（比如 Mocha 或 Jest）提供的断言库来代替 assert 方法。
- 测试用例中的语法看起来正确，但是我们需要更多上下文信息才能判断这些测试是否足够覆盖所有分支和角色。
- 如果可能的话，最好写一些 E2E 测试来检查整个系统的行为，并确保这些测试包括对应的单元测试。
- 如果应用程序规模较大，请考虑维护一个日志和错误跟踪系统，以便在出现问题时轻松排查 Bug。
[skip ci] ## [3.17.0](v3.16.0...v3.17.0) (2023-04-25) ### Features * add source registry name in manifest ([#448](#448)) ([f891aed](f891aed))
|
🎉 This PR is included in version 3.17.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
