feat(selectivity): implement 'selectivity-merge-dumps' command#1220
feat(selectivity): implement 'selectivity-merge-dumps' command#1220KuznetsovRoman merged 1 commit intomasterfrom
Conversation
commit: |
KuznetsovRoman
force-pushed
the
TESTPLANE-931.merge_selectivity
branch
from
667707c to
9472064
Compare
| ); | ||
| }); | ||
|
|
||
| it("should throw if same file key has different hashes across sources", async () => { |
There was a problem hiding this comment.
Next 3 tests are similar - its easier to read them like this instead of array-generated 3 tests
| }), | ||
| ); | ||
|
|
||
| await mergeHashes(destAbsolutePath, srcAbsolutePaths, preferredCompression); |
There was a problem hiding this comment.
Not running "mergeHashes" and "mergeTests" in parallel because "mergeHashes" checks for chunks compatibility
| const depType = dependencyType as keyof NormalizedDependencies; | ||
| const depSet = new Set(result[browserId][dependencyScope][depType]); | ||
|
|
||
| result[browserId][dependencyScope][depType] = Array.from(depSet).sort((a, b) => a.localeCompare(b)); |
There was a problem hiding this comment.
Sorting object properties as well as array values to ensure consistency
| allSrcTests.forEach(fileBase => { | ||
| const fileProviders = srcTestPaths.filter((_, idx) => srcTests[idx].has(fileBase)); | ||
|
|
||
| if (fileProviders.length === 1) { |
There was a problem hiding this comment.
If only one chunk has test dependencies, we can skip decompressing/reading/compressing/writing it and just copy the file
KuznetsovRoman
force-pushed
the
TESTPLANE-931.merge_selectivity
branch
2 times, most recently
from
03d9ba3 to
b0998e9
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0998e93f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
KuznetsovRoman
force-pushed
the
TESTPLANE-931.merge_selectivity
branch
from
b0998e9 to
1440ea6
Compare
| await Promise.all( | ||
| srcAbsolutePaths.map(srcAbsolutePath => { | ||
| return fs.promises.access(srcAbsolutePath, fs.constants.R_OK).catch(cause => { | ||
| throw new Error(`Couldn't get read access to source directory "${srcAbsolutePath}"`, { cause }); |
There was a problem hiding this comment.
not actionable error. what should I do with this error?
There was a problem hiding this comment.
Will handle "access" errors in next PR in order to make it universal
| throw new Error( | ||
| [ | ||
| `Can't merge reports: no suitable source file was found by "${sourceBasePath}" base`, | ||
| `It can happen if it was compressed with unsupported compression type`, |
There was a problem hiding this comment.
Changed to:
`Can't merge reports: no suitable source file was found by "${sourceBasePath}" base`,
`It can happen if it was compressed with unsupported compression type, or if the file was removed during command run`,
"If selectivity dump was created with node.js v22+, ensure you are currently is using it too",
Does not seem like there could be other reasons
|
|
||
| for (const dependencyType in testContent[browserId][dependencyScope]) { | ||
| const depType = dependencyType as keyof NormalizedDependencies; | ||
| for (const dependency of testContent[browserId][dependencyScope][depType]) { |
There was a problem hiding this comment.
mmm... x5 for...we should go deeper :)
| })); | ||
| readJsonWithCompressionStub = sandbox.stub().resolves({}); | ||
| stripCompressionSuffixStub = sandbox.stub().callsFake((file: string) => { | ||
| for (const suffix of [".gz", ".br", ".zstd"]) { |
There was a problem hiding this comment.
we don't need this logic here. it should be tested in the separate utils test
There was a problem hiding this comment.
Its not tested here.
Its just a stub, which makes it easier to write other tests, without necessity of stubbing it manually in each unit test
| }); | ||
| writeJsonWithCompressionStub = sandbox.stub().resolves(); | ||
|
|
||
| mergeTests = proxyquire("src/browser/cdp/selectivity/merge-dumps/merge-tests", { |
There was a problem hiding this comment.
it looks like integration tests would be better here
There was a problem hiding this comment.
Probably. Still, unit tests are fast and stable.
If needed, intergration tests can be written too, but units are still useful here: they prevent from next changes breaking existing functionality
KuznetsovRoman
force-pushed
the
TESTPLANE-931.merge_selectivity
branch
from
1440ea6 to
70fd3ba
Compare
2 participants
No description provided.