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.

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

test: [M3-8688, M3-9039] - Autocomplete component tests #11408

Merged
merged 7 commits into from
Jan 7, 2025

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Dec 12, 2024

Description 📝

Adds component tests for autocomplete component

How to test 🧪

  • mainly been using yarn cy:debug and selecting component tests to check these tests
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

import { checkComponentA11y } from 'support/util/accessibility';
import { componentTests, visualTests } from 'support/util/components';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only eslint changes in this file and region-select.spec.tsx

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for taking care of that!

@@ -0,0 +1,685 @@
import { Autocomplete } from '@linode/ui';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we've decided to go with cypress component tests, should we now move everything out of the poc package?

Copy link
Contributor

Choose a reason for hiding this comment

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

@coliu-akamai we definitely should! I just opened M3-9039 for this (not well refined right now), but you're welcome to move this spec now or we can just handle it with M3-9039.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the heads up! created a components package in place of poc to put everything in (since there's an existing features package) - would it make sense to create individual packages for each component as well? I didn't yet bc those packages would all just have one file

also would we want to create a ui package for the components that have been moved to linode/ui? 😅

@coliu-akamai coliu-akamai marked this pull request as ready for review December 12, 2024 22:20
@coliu-akamai coliu-akamai requested a review from a team as a code owner December 12, 2024 22:20
@coliu-akamai coliu-akamai requested review from jdamore-linode and removed request for a team December 12, 2024 22:20
@coliu-akamai coliu-akamai requested a review from a team as a code owner December 12, 2024 22:21
@coliu-akamai coliu-akamai requested review from dwiley-akamai and bnussman-akamai and removed request for a team December 12, 2024 22:21
Copy link

github-actions bot commented Dec 12, 2024

Coverage Report:
Base Coverage: 86.94%
Current Coverage: 86.96%

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Getting this when I try running the test 🤔

Screenshot 2024-12-13 at 2 59 06 PM

@coliu-akamai
Copy link
Contributor Author

@dwiley-akamai hmm I don't seem to be getting that issue when I run any of the component tests - is this happening only for autocomplete, or does it also happen for beta-chip and region-select?

@jdamore-linode any chance you know what might be causing this?

@jdamore-linode
Copy link
Contributor

@coliu-akamai I haven't seen it while testing your PR locally, but I have seen it intermittently on other branches, in CI, etc. When I've experienced it, it's always been a fluke and has succeeded on reattempts, but curious if @dwiley-akamai is seeing this consistently. Happy to take a closer look if so!

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Thanks @coliu-akamai, these tests look really great!

Comment on lines 427 to 447
// figure out how to confirm multi selections
// input value doesn't work anymore... (this feels hacky)
const MultiSelect = () => {
const [selectedOptions, setSelectedOptions] = React.useState<
Option[]
>([]);
return (
<>
<div>Number of selected options: {selectedOptions.length}</div>
<Autocomplete
label="Linodes"
multiple
onChange={(_, value) => setSelectedOptions(value)}
options={options}
value={selectedOptions}
/>
</>
);
};

mount(<MultiSelect />);
Copy link
Contributor

Choose a reason for hiding this comment

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

We're still trying to figure out guidelines and conventions for these tests, but at a glance I think this is a smart and pretty clear way to ensure that the Autocomplete does what it's supposed to. Curious if anyone has any other thoughts!

import { checkComponentA11y } from 'support/util/accessibility';
import { componentTests, visualTests } from 'support/util/components';
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for taking care of that!

@dwiley-akamai
Copy link
Contributor

It's still happening unfortunately and it is happening for all three on my end @coliu-akamai @jdamore-linode

Screenshots

Screenshot 2024-12-18 at 5 23 13 PM

Screenshot 2024-12-18 at 5 23 19 PM

Screenshot 2024-12-18 at 5 23 26 PM

Confirmed that it is only the Component Specs failing to run though (not E2E Specs). Are you aware of any config adjustments needed to run these successfully? Maybe I'm missing something locally

@coliu-akamai coliu-akamai changed the title test: [M3-8688] - Autocomplete component tests test: [M3-8688, M3-9039] - Autocomplete component tests Dec 19, 2024
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 471 passing tests on test run #11 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing471 Passing2 Skipped92m 50s

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 2, 2025
@coliu-akamai
Copy link
Contributor Author

@dwiley-akamai - I don't remember having any config adjustments, but can't say for sure... @jdamore-linode do you remember?

creating a follow up ticket [M3-9062] to investigate further if needed

@bnussman-akamai
Copy link
Member

No issues on my end when testing with yarn cy:debug

@jdamore-linode
Copy link
Contributor

Ah I'm sorry, I meant to take a closer look at this @coliu-akamai and never did. It doesn't sound like this is specific to your Autocomplete tests though so I don't think it should hold up merging this

@coliu-akamai
Copy link
Contributor Author

sounds good, will be merging - thanks for the help everyone

@coliu-akamai coliu-akamai merged commit 2e4dabf into linode:develop Jan 7, 2025
23 checks passed
Copy link

cypress bot commented Jan 7, 2025

Cloud Manager E2E    Run #7043

Run Properties:  status check passed Passed #7043  •  git commit 2e4dabfc31: test: [M3-8688, M3-9039] - Autocomplete component tests (#11408)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #7043
Run duration 29m 12s
Commit git commit 2e4dabfc31: test: [M3-8688, M3-9039] - Autocomplete component tests (#11408)
Committer Connie Liu
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 474
View all changes introduced in this branch ↗︎

dmcintyr-akamai pushed a commit to dmcintyr-akamai/manager that referenced this pull request Jan 9, 2025
* autocomplete tests

* eslint

* cleanup tests

* Added changeset: Add cypress component tests for Autocomplete

* address feedback @jdamore-linode

* Update packages/manager/cypress/component/poc/autocomplete.spec.tsx

Co-authored-by: jdamore-linode <97627410+jdamore-linode@users.noreply.github.com>

* create components package within ...component... package

---------

Co-authored-by: jdamore-linode <97627410+jdamore-linode@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants