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 Document list test file to new format #3446

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

SriHV
Copy link
Contributor

@SriHV SriHV commented Dec 3, 2024

What is the context of this PR?

46

Refactored the document list macro.spec file as per the new format.

How to review this PR

Ask yourself....

  • Do we need any additional tests?
  • Are the tests descriptions clear?
  • Does the order of the test document make sense?
  • Is it internally consistent?
  • Is it consistent with the guidelines for test refactoring?

run this test command to view the test results:
yarn test --testNamePattern="FOR: Macro: Document list"

Checklist

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

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

@SriHV SriHV self-assigned this Dec 3, 2024
Copy link

netlify bot commented Dec 3, 2024

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

Name Link
🔨 Latest commit 79cf5bd
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/67814c2cfce6bb0008d412cb
😎 Deploy Preview https://deploy-preview-3446--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.

@SriHV SriHV requested a review from a team December 3, 2024 12:42
@SriHV SriHV added the Enhancement Change of existing feature label Dec 3, 2024
@SriHV SriHV marked this pull request as ready for review December 3, 2024 12:43
@rmccar rmccar changed the title Refactor Document list to new format Refactor Document list test file to new format Dec 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.

Some minor comments but the THENs have been missed from this file

src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
const hiddenText = $('.ons-document-list__item-attribute').text().trim();
expect(hiddenText).toBe('PDF, 499KB, 1 page');
test('THEN: has file information displayed', () => {
const hiddenText = $('.ons-document-list__item-attribute').text().trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called hiddenText? isn't it testing for text that is displayed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is testing displayed text not hidden text so should be named as such

@rmccar
Copy link
Contributor

rmccar commented Dec 5, 2024

Can you just have a double check that all params that can be set have a test that tests their functionality. You can use the _macro-options.md file for a list

@SriHV
Copy link
Contributor Author

SriHV commented Dec 9, 2024

Can you just have a double check that all params that can be set have a test that tests their functionality. You can use the _macro-options.md file for a list

Have gone through the documentation and added few tests. I think all params are being covered now.

@rmccar
Copy link
Contributor

rmccar commented Dec 19, 2024

Can you just have a double check that all params that can be set have a test that tests their functionality. You can use the _macro-options.md file for a list

Have gone through the documentation and added few tests. I think all params are being covered now.

There are still some tests missing, I started going through them and noticed attributes isn't covered and classes is covered at the top level but not at the document level. I haven't investigated any further so can you have another look through them and make sure there aren't others that have been missed

src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
src/components/document-list/_macro.spec.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Change of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants