You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "SBIN2010 (via GitHub)" <gi...@apache.org> on 2025/10/01 22:04:59 UTC
[PR] fix(test): changed test use unsaved changes prompt [superset]
SBIN2010 opened a new pull request, #35447:
URL: https://github.com/apache/superset/pull/35447
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
After adding PR #35307, linter checks do not pass
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This is what is corrected in this
[testing guidelines](https://github.com/apache/superset/wiki/Testing-Guidelines-and-Best-Practices#avoid-nesting-when-youre-testing)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
BEFORE
<img width="695" height="218" alt="image" src="https://github.com/user-attachments/assets/39e9cee6-8b15-415d-8ef1-f1bbd104097c" />
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(test): changed test use unsaved changes prompt [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #35447:
URL: https://github.com/apache/superset/pull/35447
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(test): changed test use unsaved changes prompt [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #35447:
URL: https://github.com/apache/superset/pull/35447#discussion_r2397475057
##########
superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx:
##########
@@ -22,115 +22,116 @@ import { Router } from 'react-router-dom';
import { createMemoryHistory } from 'history';
import { act } from 'spec/helpers/testing-library';
-const history = createMemoryHistory({
+let history = createMemoryHistory({
initialEntries: ['/dashboard'],
});
+beforeEach(() => {
+ history = createMemoryHistory({ initialEntries: ['/dashboard'] });
+});
Review Comment:
beforeEach should work on a flat structure as well right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(test): changed test use unsaved changes prompt [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35447:
URL: https://github.com/apache/superset/pull/35447#discussion_r2396025685
##########
superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx:
##########
@@ -30,107 +30,104 @@ const wrapper = ({ children }: { children: React.ReactNode }) => (
<Router history={history}>{children}</Router>
);
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
-describe('useUnsavedChangesPrompt', () => {
- test('should not show modal initially', () => {
- const { result } = renderHook(
- () =>
- useUnsavedChangesPrompt({
- hasUnsavedChanges: true,
- onSave: jest.fn(),
- }),
- { wrapper },
- );
-
- expect(result.current.showModal).toBe(false);
- });
+test('should not show modal initially', () => {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test isolation failure</b></div>
<div id="fix">
The PR removes the describe block wrapper and converts tests to standalone test functions, but this creates a critical test isolation issue. All tests now share the same `history` instance and `wrapper` configuration, which can cause test pollution where state from one test affects subsequent tests. The `history` object accumulates navigation state across tests, and the shared `wrapper` component may retain state. This will lead to flaky tests and false positives/negatives in CI/CD pipelines. Add proper test isolation by reinitializing the history instance before each test using `beforeEach`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
-
-const history = createMemoryHistory({
- initialEntries: ['/dashboard'],
-});
-
+let history = createMemoryHistory({
+ initialEntries: ['/dashboard'],
+});
+
+beforeEach(() => {
+ history = createMemoryHistory({ initialEntries: ['/dashboard'] });
+});
+
```
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35447#issuecomment-3358322061>#c65c3d</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(test): changed test use unsaved changes prompt [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35447:
URL: https://github.com/apache/superset/pull/35447#issuecomment-3358322061
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(test): changed test use unsaved changes prompt [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #35447:
URL: https://github.com/apache/superset/pull/35447#discussion_r2397559420
##########
superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx:
##########
@@ -22,115 +22,116 @@ import { Router } from 'react-router-dom';
import { createMemoryHistory } from 'history';
import { act } from 'spec/helpers/testing-library';
-const history = createMemoryHistory({
+let history = createMemoryHistory({
initialEntries: ['/dashboard'],
});
+beforeEach(() => {
+ history = createMemoryHistory({ initialEntries: ['/dashboard'] });
+});
Review Comment:
Yes, beforeEach works perfectly fine with a flat test structure.
It runs before each test - every test gets a fresh history instance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(test): changed test use unsaved changes prompt [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #35447:
URL: https://github.com/apache/superset/pull/35447#discussion_r2399344590
##########
superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx:
##########
@@ -22,115 +22,116 @@ import { Router } from 'react-router-dom';
import { createMemoryHistory } from 'history';
import { act } from 'spec/helpers/testing-library';
-const history = createMemoryHistory({
+let history = createMemoryHistory({
initialEntries: ['/dashboard'],
});
+beforeEach(() => {
+ history = createMemoryHistory({ initialEntries: ['/dashboard'] });
+});
Review Comment:
Ok, but i don't quite understand this change still? If const was working before do we need this let and reassign in beforeEach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org