Have the lint job its own cache keys #1749
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe updates include changes to the Ruby gem cache key suffixes in a GitHub Actions workflow, conditional mocking of the global Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Jest Test Runner
participant Env as Environment Variables
participant Console as global.console
TestRunner->>Env: Check ENABLE_JEST_CONSOLE
alt ENABLE_JEST_CONSOLE is not truthy
TestRunner->>Console: Log "All calls to console have been disabled..."
TestRunner->>Console: Override methods with jest.fn()
else ENABLE_JEST_CONSOLE is truthy
TestRunner->>Console: Leave as is
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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
🧹 Nitpick comments (2)
node_package/tests/jest.setup.js (2)
66-72: Consider consistency with conditional environment checks.The console mocking implementation is correct and follows Jest patterns. However, it's applied unconditionally, which differs from the conditional approach used elsewhere in this file.
Consider making this consistent with the existing pattern:
+// Mock console methods for better test control and assertions +if (typeof global !== 'undefined') { global.console = { log: jest.fn(), error: jest.fn(), warn: jest.fn(), info: jest.fn(), debug: jest.fn(), }; +}
66-72: Consider mocking additional console methods for comprehensive coverage.The current implementation only mocks 5 console methods. Consider including other commonly used console methods that tests might invoke:
global.console = { log: jest.fn(), error: jest.fn(), warn: jest.fn(), info: jest.fn(), debug: jest.fn(), + trace: jest.fn(), + time: jest.fn(), + timeEnd: jest.fn(), + group: jest.fn(), + groupEnd: jest.fn(), + groupCollapsed: jest.fn(), + table: jest.fn(), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node_package/tests/jest.setup.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (1)
node_package/tests/jest.setup.js (1)
66-72: Note: This will suppress console output during test execution.The global console mocking will prevent actual console output from appearing during test runs, which might impact debugging. Consider documenting this behavior or providing a way to conditionally enable/disable console mocking during development.
You might want to add a comment explaining this behavior and how developers can restore console for debugging if needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
node_package/tests/jest.setup.js (1)
66-66: Fix formatting issues.ESLint/Prettier has flagged formatting inconsistencies that need to be addressed.
-global.console.log("All calls to console have been disabled in jest.setup.js") +global.console.log('All calls to console have been disabled in jest.setup.js');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/lint-js-and-ruby.yml(2 hunks)node_package/tests/jest.setup.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/lint-js-and-ruby.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
🪛 ESLint
node_package/tests/jest.setup.js
[error] 66-66: Replace "All·calls·to·console·have·been·disabled·in·jest.setup.js") with 'All·calls·to·console·have·been·disabled·in·jest.setup.js');
(prettier/prettier)
[error] 74-74: Insert ;
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
node_package/tests/jest.setup.js
Outdated
Comment on lines
68
to
74
| global.console = { | ||
| log: jest.fn(), | ||
| error: jest.fn(), | ||
| warn: jest.fn(), | ||
| info: jest.fn(), | ||
| debug: jest.fn(), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve console mocking completeness and fix formatting.
The current implementation only mocks 5 console methods, but console has many more methods (assert, clear, count, dir, group, table, time, trace, etc.). This partial mocking could lead to inconsistent behavior if other console methods are used.
-global.console = {
- log: jest.fn(),
- error: jest.fn(),
- warn: jest.fn(),
- info: jest.fn(),
- debug: jest.fn(),
-}
+// Create a comprehensive mock of all console methods
+global.console = Object.keys(console).reduce((mockConsole, method) => {
+ mockConsole[method] = jest.fn();
+ return mockConsole;
+}, {});Alternatively, if you want to be more selective, consider using Jest's built-in console mocking:
-global.console = {
- log: jest.fn(),
- error: jest.fn(),
- warn: jest.fn(),
- info: jest.fn(),
- debug: jest.fn(),
-}
+// Mock console globally but allow restoration if needed
+jest.spyOn(console, 'log').mockImplementation(() => {});
+jest.spyOn(console, 'error').mockImplementation(() => {});
+jest.spyOn(console, 'warn').mockImplementation(() => {});
+jest.spyOn(console, 'info').mockImplementation(() => {});
+jest.spyOn(console, 'debug').mockImplementation(() => {});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| global.console = { | |
| log: jest.fn(), | |
| error: jest.fn(), | |
| warn: jest.fn(), | |
| info: jest.fn(), | |
| debug: jest.fn(), | |
| } | |
| // Create a comprehensive mock of all console methods | |
| global.console = Object.keys(console).reduce((mockConsole, method) => { | |
| mockConsole[method] = jest.fn(); | |
| return mockConsole; | |
| }, {}); |
🧰 Tools
🪛 ESLint
[error] 74-74: Insert ;
(prettier/prettier)
🤖 Prompt for AI Agents
In node_package/tests/jest.setup.js around lines 68 to 74, the global console
mock only includes five methods, missing many others like assert, clear, count,
dir, group, table, time, and trace, which can cause inconsistent test behavior.
To fix this, extend the mock to include all commonly used console methods or
replace the entire global.console with Jest's built-in console mocking utility
to ensure comprehensive and consistent mocking across all console methods.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
node_package/tests/jest.setup.js(1 hunks)node_package/tests/renderFunction.test.jsx(1 hunks)spec/dummy/tests/react-on-rails.import.test.js(1 hunks)spec/dummy/tests/react-on-rails.require.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- node_package/tests/jest.setup.js
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
node_package/tests/renderFunction.test.jsx (1)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
spec/dummy/tests/react-on-rails.import.test.js (10)
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
spec/dummy/tests/react-on-rails.require.test.js (12)
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
🪛 GitHub Actions: JS unit tests for Renderer package
node_package/tests/renderFunction.test.jsx
[error] 18-18: Test failure: expect(received).toBe(expected) failed. Expected false but received true in 'isRenderFunction › returns false for a ES5 React Component'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (2)
spec/dummy/tests/react-on-rails.import.test.js (1)
4-6: Excellent improvement to test assertion clarity!Making the assertion explicit with
expect().not.toThrow()is a testing best practice that clearly communicates the test's intent and will provide better error messages ifReactOnRails.register({})starts throwing exceptions.spec/dummy/tests/react-on-rails.require.test.js (1)
4-6: Great consistency with the import test!This change mirrors the improvement made in
react-on-rails.import.test.js, ensuring both ES6 import and CommonJS require loading methods have equivalent explicit assertions. The consistent application of testing best practices across both files is commendable.
Judahmeek
changed the title
alexeyr-ci2
requested changes
Jul 13, 2025
| with: | ||
| path: vendor/bundle | ||
| key: package-app-gem-cache-${{ hashFiles('Gemfile.lock') }}-oldest | ||
| key: package-app-gem-cache-${{ hashFiles('Gemfile.lock') }}-lint |
There was a problem hiding this comment.
Therefore, we should switch to using the newest spec/dummy gem cache.
Newest makes sense, but a separate -lint one doesn't to me.
There was a problem hiding this comment.
newest didn't work. That's why one of the commits is named "newest cache is still borked". It results in the gems being reinstalled just like the oldest cache key did.
The newest cache key works for other jobs, like Rspec, so I have no idea what is wrong.
All I know is that the lint key fixes the problem.
node_package/tests/jest.setup.js
Outdated
| global.ReadableStreamDefaultReader = ReadableStreamDefaultReader; | ||
| } | ||
|
|
||
| global.console.log('All calls to console have been disabled in jest.setup.js'); |
There was a problem hiding this comment.
Why? It's useful to see logs when running tests. Error/warning messages from React and other libraries in particular.
At least make an environment variable to disable this.
There was a problem hiding this comment.
The logs make it difficult to distinguish which test is failing and which console error is connected to the failure.
This change clears out all console output from the passing tests. Which makes seeing the output that actually matters a lot easier.
Happy to add an environment variable if you think it helps.
There was a problem hiding this comment.
I don't see why it would only clear console output from passing tests. If it does leave it in place for failing tests, great.
alexeyr-ci2
approved these changes
Jul 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the lint job is trying to use the oldest spec/dummy's gem cache without running the conversion script. This causes every gem that would be removed/modified by the conversion script to instead be installed every time the job runs.
However, running the conversion script would cause eslint to be removed.
Therefore, we should switch to using the newest spec/dummy gem cache.
This change is
Summary by CodeRabbit
Tests
Chores