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

Add tests for KTextbox #322

Merged
merged 4 commits into from
Mar 17, 2022
Merged

Conversation

sairina
Copy link
Contributor

@sairina sairina commented Mar 9, 2022

Description

  • Adds tests for KTextbox
  • Cleans up some of naming of the test folders in KDS
  • Adds test as part of the changelog

Issue addressed

Fixes #306

Steps to test

  1. yarn run test

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • Critical and brittle code paths are covered by unit tests
  • The change has been added to the changelog

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?

Post-merge updates and tracking

  • Learning Platform update PR is submitted
  • Learning Platform update PR is merged
  • Studio update PR is submitted
  • Studio update PR is merged
  • Data Portal update PR is submitted
  • Data Portal update PR is merged

Comments

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

This looks good to me! I did have one question, but I think it's maybe my own confusion around writing tests using 'attribute'. I don't see any issues and all of the tests are passing. Maybe @nucleogenesis can weigh in on if there is anything else he'd like to see, but otherwise looks good to me.

disabled: true,
},
});
expect(wrapper.find('input').attributes('disabled')).toBe('disabled');
Copy link
Member

Choose a reason for hiding this comment

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

I don't totally understand why this would be 'disabled' and not a boolean, although the test is passing

Copy link
Member

Choose a reason for hiding this comment

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

disabled is only boolean by virtue of its presence or absence - that's not always been supported, if I recall correctly, so it's often been conventional to set the value to 'disabled'.

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL it has something to do with xhtml: https://stackoverflow.com/a/6531804

Copy link
Member

Choose a reason for hiding this comment

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

Ahah - thanks for the canonical explanation!

Comment on lines +30 to +51
it(`invalidText is not displayed if showInvalidText is false`, () => {
const wrapper = mount(KTextbox, {
propsData: {
invalid: true,
showInvalidText: false,
invalidText: 'error!',
},
});
const errorTextField = wrapper.find('.ui-textbox-feedback-text');
expect(errorTextField.text()).not.toBe('error!');
});
it(`invalidText is displayed through error prop if invalid and showInvalidText are both true`, () => {
const wrapper = mount(KTextbox, {
propsData: {
invalid: true,
showInvalidText: true,
invalidText: 'error!',
},
});
const errorTextField = wrapper.find('.ui-textbox-feedback-text');
expect(errorTextField.text()).toBe('error!');
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm so sorry for this weird set of props for handling invalid states but glad to have tests added for it!

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

This all looks great! Thanks Sairina!

@sairina sairina merged commit 28111df into learningequality:main Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write unit tests for KTextbox
4 participants