fix: adjust the meaning of logUploadIntervalMultiplicationFactor #4034
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
marcin-bazyl
added
the
validated
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint
2523-3024: Test coverage for error handling could be improvedWhile there is good test coverage for the happy path, some error scenarios could use more testing:
- Network errors during media negotiation
- Timeout scenarios
- Race conditions between multiple operations
- Error recovery and retry logic
Consider adding tests for:
// Test network errors it('handles network errors during media negotiation', async () => { // Setup meeting.mediaProperties.webrtcMediaConnection.initiateOffer.rejects(new Error('Network error')); // Test await assert.isRejected(meeting.addMedia()); // Verify error handling and cleanup assert.isNull(meeting.mediaProperties.webrtcMediaConnection); assert.calledOnce(meeting.cleanupLocalStreams); }); // Test timeout scenarios it('handles timeouts during media connection', async () => { const clock = sinon.useFakeTimers(); // Setup timeout condition meeting.mediaProperties.waitForMediaConnectionConnected.returns(new Promise(() => {})); // Start operation const promise = meeting.addMedia(); // Advance clock past timeout await clock.tickAsync(ICE_AND_DTLS_CONNECTION_TIMEOUT + 100); // Verify timeout handling await assert.isRejected(promise); assert.calledOnce(meeting.cleanupLocalStreams); });
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (3)
Line range hint
5-65: Consider grouping related imports togetherThe imports section could be better organized by grouping related imports together:
- External libraries (lodash, sinon etc.)
- Internal modules (@webex/*)
- Test helpers
- Constants and types
- Main modules being tested
// External libraries import 'jsdom-global/register'; import {cloneDeep, forEach, isEqual, isUndefined} from 'lodash'; import sinon from 'sinon'; import {assert, expect} from '@webex/test-helper-chai'; // Internal core modules import {Credentials, WebexPlugin} from '@webex/webex-core'; import Support from '@webex/internal-plugin-support'; import MockWebex from '@webex/test-helper-mock-webex'; // Internal media modules import * as InternalMediaCoreModule from '@webex/internal-media-core'; import * as RemoteMediaManagerModule from '@webex/plugin-meetings/src/multistream/remoteMediaManager'; // Internal meetings modules import StaticConfig from '@webex/plugin-meetings/src/common/config'; import ReconnectionNotStartedError from '@webex/plugin-meetings/src/common/errors/reconnection-not-started'; // Rest of the imports...
2495-2516: Consider extracting test helper functionThe log counter check logic is repeated multiple times. Consider extracting it into a helper function for better readability and maintainability.
+ const verifyLogCounter = (clock, logUploadCounter, delayInMinutes, expectedCounter) => { + const delayInMilliseconds = delayInMinutes * 60 * 1000; + + // Check counter before delay + clock.tick(delayInMilliseconds - 50); + assert.equal(logUploadCounter, expectedCounter - 1); + + // Check counter after delay + clock.tick(50); + assert.equal(logUploadCounter, expectedCounter); + }; - checkLogCounter(0.1, 1); - checkLogCounter(1, 2); - checkLogCounter(15, 3); - checkLogCounter(15, 4); - checkLogCounter(30, 5); - checkLogCounter(30, 6); - checkLogCounter(30, 7); - checkLogCounter(60, 8); - checkLogCounter(60, 9); + verifyLogCounter(clock, logUploadCounter, 0.1, 1); + verifyLogCounter(clock, logUploadCounter, 1, 2); + verifyLogCounter(clock, logUploadCounter, 15, 3); + verifyLogCounter(clock, logUploadCounter, 15, 4); + verifyLogCounter(clock, logUploadCounter, 30, 5); + verifyLogCounter(clock, logUploadCounter, 30, 6); + verifyLogCounter(clock, logUploadCounter, 30, 7); + verifyLogCounter(clock, logUploadCounter, 60, 8); + verifyLogCounter(clock, logUploadCounter, 60, 9);
2517-2522: Consider adding more assertions for media connection cleanupThe test for media connection cleanup could be more thorough by verifying that all resources are properly released.
meeting.mediaProperties.webrtcMediaConnection = undefined; checkLogCounter(60, 10); clock.tick(120*1000*60); assert.equal(logUploadCounter, 10); + // Verify cleanup + assert.isUndefined(meeting.mediaProperties.webrtcMediaConnection); + assert.isNull(meeting.statsAnalyzer); + assert.isNull(meeting.reconnectionManager);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/@webex/plugin-meetings/src/config.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/@webex/plugin-meetings/src/config.ts
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
4102-4106:
Fix the reference to the correct array variable.
There is a critical typo: "LOG_UPLOAD_INTERVAL_INTERVALS" is referenced instead of "LOG_UPLOAD_INTERVALS". This will break the logic of calculating the delay.
Apply this diff to fix the incorrect reference:
- LOG_UPLOAD_INTERVAL_INTERVALS[this.logUploadIntervalIndex];
+ LOG_UPLOAD_INTERVALS[this.logUploadIntervalIndex];Likely invalid or redundant comment.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (2)
Line range hint 1-4: LGTM: Copyright and imports look good
The file starts with appropriate copyright notice and imports the required test dependencies.
Line range hint 66-2494: Test structure and coverage look good
The test file has good coverage of the Meeting class functionality with well-organized test suites for:
- Constructor and initialization
- Public API methods (join, leave, media handling etc.)
- Private methods and helpers
- Event handling
- Error cases
The tests use appropriate mocking and stubs to isolate the code under test.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
4102-4102: Clarify the “6-second” first interval in comments.
The use of 0.1 minutes (which translates to 6 seconds) in the LOG_UPLOAD_INTERVALS array works well to delay the initial upload, but may not be immediately obvious. Consider updating or adding a comment explaining that 0.1 was chosen to represent 6 seconds.packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
2495-2516: Improve readability of log upload interval test casesThe log upload interval test cases use magic numbers for delays without clear documentation of what they represent. Consider:
- Extracting the delay values into named constants that describe their purpose
- Adding comments explaining the expected behavior at each interval
- Using more descriptive variable names than just
checkLogCounter- checkLogCounter(0.1, 1); - checkLogCounter(1, 2); - checkLogCounter(15, 3); - checkLogCounter(15, 4); - checkLogCounter(30, 5); - checkLogCounter(30, 6); - checkLogCounter(30, 7); - checkLogCounter(60, 8); - checkLogCounter(60, 9); + // Define expected upload intervals + const INITIAL_DELAY = 0.1; // 6 seconds + const FIRST_INTERVAL = 1; // 1 minute + const SECOND_INTERVAL = 15; // 15 minutes + const THIRD_INTERVAL = 30; // 30 minutes + const FINAL_INTERVAL = 60; // 60 minutes + // Verify log uploads at each interval + verifyLogUpload(INITIAL_DELAY, 1); // First upload after 6 seconds + verifyLogUpload(FIRST_INTERVAL, 2); // Second upload after 1 minute + verifyLogUpload(SECOND_INTERVAL, 3); // Third upload after 15 minutes + verifyLogUpload(SECOND_INTERVAL, 4); // Fourth upload after 15 minutes + verifyLogUpload(THIRD_INTERVAL, 5); // Fifth upload after 30 minutes + verifyLogUpload(THIRD_INTERVAL, 6); // Sixth upload after 30 minutes + verifyLogUpload(THIRD_INTERVAL, 7); // Seventh upload after 30 minutes + verifyLogUpload(FINAL_INTERVAL, 8); // Eighth upload after 60 minutes + verifyLogUpload(FINAL_INTERVAL, 9); // Ninth upload after 60 minutes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/@webex/plugin-meetings/src/config.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/@webex/plugin-meetings/src/config.ts
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
4106-4106: Validate that shorter and subsequent longer intervals behave as intended.
Although 0.1 minutes for the first upload is likely to reduce initial excessive requests, ensure that the subsequent intervals (1, 15, etc.) and the scaling via logUploadIntervalMultiplicationFactor are thoroughly tested to avoid regressions and 429 issues.
packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js (1)
Line range hint 1-3900: LGTM! Well structured and comprehensive test suite
The test file demonstrates good testing practices:
- Comprehensive test coverage of the Meeting class functionality
- Well organized using describe blocks and clear test names
- Proper test setup/teardown with beforeEach/afterEach hooks
- Good mocking and stubbing of dependencies
- Edge cases and error scenarios are properly tested
- Effective use of sinon for mocking and test utilities
This pull request addresses
If some client sets the
logUploadIntervalMultiplicationFactorto 1, then logs will be uploaded periodically every few seconds, which is a bit too much and causes 429 errors from the backendby making the following changes
changed the code so that having
logUploadIntervalMultiplicationFactor: 1will mean that logs get uploaded initially after 6s, then after another 15 minutes, 30 min and every 60 min after thatlogUploadIntervalMultiplicationFactoris a new config that was recently added and is not used by any app, yet, so it's OK to change its meaning.Change Type
The following scenarios were tested
existing unit tests and tested manually with web app
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests