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

Feature/create unit tests #9

Merged

Conversation

Luciaisacomputer
Copy link
Contributor

What is this PR for?

This adds two tests to the project, a unit test for testing the instantiation of Announcer, and an a11y test which uses Axe to test a couple of a features.

Where should the reviewer start?

  1. New dependencies means you'll need to run npm install
  2. Next you need to run npm test

What should the reviewer look at?

  1. When you run the test command two tests are ran together:

test:jest which runs the unit test to make sure the component is created, has default props, and that the text node which the aria-live node depends on updates correctly.

test:a11y which loads the basic demo example but with the Axe suite running, this checks a few properties to make sure they are correctly used. (I wasn't really sure where else to go with this, so I am open to suggestions).

  1. Open the browser and make sure there are no errors in the dev tools console

  2. In your terminal make sure all three tests have passed

What gif best describes this PR or how it makes you feel?

Lego Jester

Additional comments:

This was certainly a very tricky piece to work on. Jest made things a lot easier after I took a few moments to read through the docs. It seems to be a little more opinionated since it is designed to work with React, but I think that helps a lot.

For the actual a11y testing, I have a few checks in place, but we may want to see if there are other automatic ways to test what we are doing. On the Jest end, we can check the text values inside of the components node when the prop for text changes which lets us know that it should update on the front-end, but that doesn't actually handle the announcing as you would hear from a screen reader.

I almost wonder if we could use speech synthesis of some kind to have an easy way to test the announcing, for example the app could load, and then speech synthesis would read the text value as it changes. It wouldn't be a replacement for testing on actual devices, but might make working with it a little easier.

Definition of Done:

  • [✅] Is there appropriate test coverage?
  • [✅] Does this pass all automatic linting?
  • [✅] Does the documentation need to be updated?
  • [✅] Does this add new dependencies?

@tikaro
Copy link
Contributor

tikaro commented Aug 9, 2017

Here's a general question: Sometimes I see instructions to say npm run COMMAND, and sometimes i see instructions to say npm COMMAND. Is it the case that some commands (like "install") are generic, and don't require run, and some are special-purpose scripts, and do?

I was kind of expecting to see npm run test.

@tikaro
Copy link
Contributor

tikaro commented Aug 9, 2017

When I ran npm run test:a11y, then visited the

serious: <html> element must have a lang attribute https://dequeuniversity.com/rules/axe/2.3/html-has-lang?application=axeAPI

@tikaro
Copy link
Contributor

tikaro commented Aug 9, 2017

Here's what we decided to do about this:

  1. Keep the unit tests, because those are working and uncontroversial, and replace a stub
  2. Back out of the axe tests because they are more complicated and require some conversation

Maybe Luke will create a new branch to hold JUST the axe stuff, and a new ticket to describe it, and we will declare victory.

Copy link
Contributor

@tikaro tikaro left a comment

Choose a reason for hiding this comment

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

Thanks, Luke! We decided to move forward with the easy half of this PR.

@pezfish
Copy link
Contributor

pezfish commented Aug 9, 2017

@tikaro npm does have some reserved words that don't require npm run, I think you most often see them with start and test https://docs.npmjs.com/misc/scripts

…s ticket and right now it isn't doing very much
@Luciaisacomputer
Copy link
Contributor Author

@tikaro I updated the code to not use the Axe library for now. It's good that we doubled down on this because I was having some issues with webpack, we may want to break out our config at some point, especially when we start having different directories for test/build/etc.

@tikaro tikaro merged commit 3297a36 into thinkcompany:master Aug 11, 2017
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.

3 participants