Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: [M3-8602] - Fix plan-selection.spec.ts test failures on retries #10976

Merged
merged 5 commits into from
Sep 20, 2024
Merged

fix: [M3-8602] - Fix plan-selection.spec.ts test failures on retries #10976

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a90aba8
WIP experimental fix
Sep 19, 2024
a2dc516
Comment explanation, better const name
Sep 19, 2024
4d1de76
Prevent plan selection test from guaranteed failures on reattempts
Sep 19, 2024
e3f339f
Added changeset: Fix plan selection test always failing on reattempts
Sep 19, 2024
bacd56d
Merge branch 'develop' into M3-8617-cy-click-autocomplete-failures, r…
Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-10976-tests-1726778238574.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Tests
---

Fix plan selection test always failing on reattempts ([#10976](https://github.com/linode/manager/pull/10976))
6 changes: 3 additions & 3 deletions packages/manager/cypress/e2e/core/linodes/plan-selection.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ const notices = {

authenticate();
describe('displays linode plans panel based on availability', () => {
before(() => {
beforeEach(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using before here instead of beforeEach causes the mocks not to get applied on re-attempts which leads to failures.

The initial failure is caused by the cy.click() issue, but plan-selection.spec.ts failures are overrepresented in CI due to the re-attempts being guaranteed to fail.

mockGetRegions(mockRegions).as('getRegions');
mockGetLinodeTypes(mockLinodeTypes).as('getLinodeTypes');
mockGetRegionAvailability(mockRegions[0].id, mockRegionAvailability).as(
Expand Down Expand Up @@ -220,7 +220,7 @@ describe('displays linode plans panel based on availability', () => {
});

describe('displays kubernetes plans panel based on availability', () => {
before(() => {
beforeEach(() => {
mockGetRegions(mockRegions).as('getRegions');
mockGetLinodeTypes(mockLinodeTypes).as('getLinodeTypes');
mockGetRegionAvailability(mockRegions[0].id, mockRegionAvailability).as(
Expand Down Expand Up @@ -350,7 +350,7 @@ describe('displays kubernetes plans panel based on availability', () => {
});

describe('displays specific linode plans for GPU', () => {
before(() => {
beforeEach(() => {
mockGetRegions(mockRegions).as('getRegions');
mockGetLinodeTypes(mockLinodeTypes).as('getLinodeTypes');
mockGetRegionAvailability(mockRegions[0].id, mockRegionAvailability).as(
Expand Down
15 changes: 11 additions & 4 deletions packages/manager/src/components/Autocomplete/Autocomplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import CloseIcon from '@mui/icons-material/Close';
import KeyboardArrowDownIcon from '@mui/icons-material/KeyboardArrowDown';
import MuiAutocomplete from '@mui/material/Autocomplete';
import React from 'react';
import React, { useCallback } from 'react';

import { Box } from 'src/components/Box';
import { TextField } from 'src/components/TextField';
Expand All @@ -17,6 +17,7 @@ import {
import type { AutocompleteProps } from '@mui/material/Autocomplete';
import type { SxProps } from '@mui/system';
import type { TextFieldProps } from 'src/components/TextField';
import type { PopperProps } from '@mui/base';

export interface EnhancedAutocompleteProps<
T extends { label: string },
Expand Down Expand Up @@ -108,11 +109,17 @@ export const Autocomplete = <

const optionsWithSelectAll = [selectAllOption, ...options] as T[];

/* Memoize popper callback to prevent unnecessary popper re-rendering. */
const customPopper = useCallback(
(props: PopperProps) => {
return <CustomPopper {...props} sx={sxPopperComponent} />;
},
[sxPopperComponent]
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a suggestion I saw on a StackOverflow thread, but hoping that somebody with more React expertise than me could take a look at this and double check that this is a sensible solution.

It seems to make the tests very happy, and I didn't notice any regressions during my manual testing, but I'd be lying if I said I fully understand the implications of wrapping this in useCallback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Joe! Take a look at #10978. Do you think this could be a better way to approach it and would it fix the unexpected re-render / arrow function issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @bnussman-akamai!

Commented on your PR, but I think your solution is hands down better: it fixes the test failures and I'm guessing it's a lot more idiomatic / in line with the way we normally do things. Happy to either integrate your implementation into this PR or let you take the reins with #10978!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdamore-linode Do you happen to have the link to the StackOverflow thread? I'm curious why useCallback was used instead of useMemo and would be interested in reading more into it. Anyway, @bnussman-akamai provided a very good alternative and I see it's been merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carrillo-erik not on hand, no -- I could try to find it, but I was already a bit doubtful about useCallback as a solution for this, and I think Bank's solution demonstrates that it wasn't really necessary in the first place, so I don't think I'd consider the answer in that thread as authoritative in this case

return (
<MuiAutocomplete
PopperComponent={(props) => {
return <CustomPopper {...props} sx={sxPopperComponent} />;
}}
PopperComponent={customPopper}
options={
multiple && !disableSelectAll && options.length > 0
? optionsWithSelectAll
Expand Down
Loading