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

Refactor Autosuggest component test file to new format #3327

Merged

Conversation

precious-onyenaucheya-ons
Copy link
Contributor

@precious-onyenaucheya-ons precious-onyenaucheya-ons commented Sep 4, 2024

What is the context of this PR?

Fixes: ONSdigital/design-team#134
This PR involves refactoring of the autosuggest test document to follow the Given, When, Then testing approach.

How to review this PR

Functionality:

  • Run the test suite using: yarn test --testNamePattern="FOR: Macro: Autosuggest"

Does this testing refactor meet the following success criteria:

  • Clearly structured.
  • Follows naming conventions.
  • Consistent.
  • Concise.
  • Suited to our codebase.
  • Compatible with existing testing framework.

Look at the refactored code in relation to:

  • The original test document.
  • The macro file.
  • The macro options file.

Checklist

This needs to be completed by the person raising the PR.

  • I have selected the correct Assignee
  • I have linked the correct Issue

@precious-onyenaucheya-ons precious-onyenaucheya-ons added the Tech improvements Tech debt, cleanup, code standardisation etc. label Sep 4, 2024
@precious-onyenaucheya-ons precious-onyenaucheya-ons requested a review from a team September 4, 2024 12:01
@precious-onyenaucheya-ons precious-onyenaucheya-ons linked an issue Sep 4, 2024 that may be closed by this pull request
Copy link

netlify bot commented Sep 4, 2024

Deploy Preview for ons-design-system-preview ready!

Name Link
🔨 Latest commit 5dd12e6
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/674446a101bad700085788dc
😎 Deploy Preview https://deploy-preview-3327--ons-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@precious-onyenaucheya-ons precious-onyenaucheya-ons changed the title Feature/134/autosuggest testing refactor Refactor autosuggest test document Sep 4, 2024
Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

I think some more work needs to be done here, use one of the other PRs as a template. We are missing the "FOR" and "THEN" in some cases

src/components/autosuggest/_macro.spec.js Outdated Show resolved Hide resolved
src/components/autosuggest/_macro.spec.js Outdated Show resolved Hide resolved
src/components/autosuggest/_macro.spec.js Outdated Show resolved Hide resolved
src/components/autosuggest/autosuggest.spec.js Outdated Show resolved Hide resolved
@precious-onyenaucheya-ons precious-onyenaucheya-ons changed the title Refactor autosuggest test document Refactor autosuggest component test file to new format Sep 12, 2024
Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

Ive realised we are missing tests for the language and resultsThreshold params. I think we should add these

@precious-onyenaucheya-ons precious-onyenaucheya-ons force-pushed the feature/134/autosuggest-testing-refactor branch from 6096db1 to 9915198 Compare November 13, 2024 15:39
Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

I think you need to check the required the params in the documentation some of these in the required describe aren't required still. Also the "params none" describe you've added actually passes in a lot of params because it uses EXAMPLE_AUTOSUGGEST. I don't think we need this describe though, we should be testing these things with the required params because we expect those params to be passed in

src/components/autosuggest/_macro.spec.js Outdated Show resolved Hide resolved
src/components/autosuggest/_macro.spec.js Show resolved Hide resolved
@precious-onyenaucheya-ons precious-onyenaucheya-ons force-pushed the feature/134/autosuggest-testing-refactor branch from b7fe5c4 to 1f43ccc Compare November 18, 2024 12:36
Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

Few tests I thought that could be added to improve our coverage

src/components/autosuggest/_macro.spec.js Show resolved Hide resolved
src/components/autosuggest/_macro.spec.js Show resolved Hide resolved
src/components/autosuggest/_macro.spec.js Show resolved Hide resolved
src/components/autosuggest/_macro.spec.js Show resolved Hide resolved
Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

Couple of typos

src/components/autosuggest/_macro.spec.js Outdated Show resolved Hide resolved
src/components/autosuggest/_macro.spec.js Outdated Show resolved Hide resolved
@precious-onyenaucheya-ons precious-onyenaucheya-ons merged commit 52ebc37 into main Nov 25, 2024
25 checks passed
@precious-onyenaucheya-ons precious-onyenaucheya-ons deleted the feature/134/autosuggest-testing-refactor branch November 25, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tech improvements Tech debt, cleanup, code standardisation etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing refactor - Autosuggest
4 participants