Added POC test cases for Avatar.test

[amjadorfali]

Before submitting the PR:

• Does your PR reference an issue? If not, please chat to the team on Discord or GitHub before submission.
• Did you update and run tests before submission using npm run test?
• Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name

What does your PR address?

Fixes #150
Improve automated test coverage and depth

Please briefly describe your changes here.

Introduced a new standard in automated testing, although can be improved more. Since i am new to writing automated tests for a UI Library, I couldn't quietly determine how "Deep" i needed to go. Nevertheless, I tried my best to adhere to the standards that Kent C Dodds and Testing-library has set to writing automated tests.

Notes :

I took certain initiatives for the current test cases for specific reasons, you can find a reference for each change/decision i took :

• Removed props property in the config of render fn
• Why to use jest-dom
• Why to use user-event
• Why I replaced most getByTestIds and class selectors short-version - - long-version, very good read
• Prioritizing screen/dom querying
• Why I removed .toBeTruthy from most tests - The table in the link indicates that the getBy... automatically throw errors when the element isn't found

Nice to haves :

• Accessibility testing - Jest-axe
• Visual Regression Testing - jest-image-snapshot
• Better handling of testing Styles - Jest-dom
• Better handling of testing User Interactions - testing-library/userEvent
• Performance testing - Couldn't find a svelte example

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
skeleton-docs	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 26, 2023 at 8:28PM (UTC)

[endigo9740]

@amjadorfali just a heads up - I haven't forgot about your PR here. I'm still just heads down prepping on the documentation side. We're making good progress, so I hope to circle back later this week. Thanks again for your patience in the meantime!

[amjadorfali]

@endigo9740 thanks for letting me know, and good luck! You guys are doing amazing work in this library.

[endigo9740]

Hey @amjadorfali, I've finally had a chance for a preliminary review. Just so you know, we're in mad sprint towards our v1.0 launch and focusing on a big series of documentation updates per:

• Documentation Updates #783

Given this, our priority is 110% documentation right now. I am interested in improving our test cases, but this is not a blocking issue for us getting v1.0 out the door. Just so you're aware of where this resides as far as priority.

Also, thank you for taking the time to discuss your influence and document your references. This looks like astounding work. This is the most well documented PR we've received, bar none!

That said, I know our current test cases are barebones. I think we can agree there. But what you've done here is like on a whole other level. I'll be honest, I'm struggling to give this a proper review. Mainly because I don't really understanding everything going on yet. If I had more bandwidth right now I'd stop, educate myself, and return to this. Unfortunately time is now allowing for that right this second.

However, before we get into specifics, I want to take a step back and reflect on the overall goal here. My knee jerk reaction is a 300 loc test case for an avatar component feels like overkill. In fact I have a few concerns here:

1. I don't want to loose the fluidity for modifying the components. We still have a few minor adjustments, bug fixes, etc that I expect we'll make over the next few months. For us, v1.0 is not a maintenance freeze, and I worry that if our test cases are too extensive we're going to have a hard time maintaining them. With every subtle change potentially triggering a failure in the tests. Fragility is always something I worry about with extensive tests.
2. As a maintainer, I need to ensure that contributions are on a level playing field, meaning anyone can contribute. Right now I don't want to handicap us into a state that you're the only one capable of understanding and maintaining our test cases. I'm not sure anyone (including myself) can keep up with this. Which would put us in a bad way.
3. And really the most important bit IMO - we have to ask ourselves how much automated testing we need and want. There very much is such a thing as over testing. We can definitely test every bit of every feature, but at some point the return in investment diminishes. We end up painting ourself into a rigid box in some ways.

Honestly I wasn't opposed to the minimal testing we had. It is essentially a litmus test of "is X component functional right now", which is a great starting point. The area I most wanted to extend to was ensuring that properties are being receive and implemented as expected. We will typically have a good idea if the basics and defaults are working browsing the live components on the docs. However, if someone is using a component and supply a background prop, but that doesn't reach it's destination within the component, that's a problem, right?

So, I'll leave it here, and give you a chance to respond. But my gut is telling me we might need to circle back to this post v1.0 and get more of a community and group consensus on how far we want to go with testing. Testing is great, extensive testing is great, but there is something to be said for "K.I.S.S", if you know what I mean.

Would you be down for us opening this up for discussion in a more official capacity in the near future?

[amjadorfali]

Hi @endigo9740, thanks for taking the time to review my PR. I'm glad to see that you've been pushing for documentation fixes lately, and I'm happy to help out with any other issues that need attention.

Regarding the tests I added, I understand your concerns about maintainability and fully agree with your points, it does seem like an overkill. However, I believe that testing UI components is important to ensure that they work as intended and don't break as changes are made. Here are some specific points to consider:

• Testing the static classes cBase and cImg may not be necessary, as these are not user-bound and may change.
• According to Kent C Dodds (he wrote @testing-library), "The more your tests resemble the way your software is used, the more confidence they can give you."
• Snapshots can be a little useful, and are easy to maintain. In my experience, I have found that you can cover all test cases (needed) without using snapshots.

If you do a quick run at any test, and. type in screen.debug() in any it block, you would see the rendered HTML printed on the console. This is very useful and recommended to plan what needs to be tested.

I am not a QA engineer, and I have much to learn about automated testing. I would love to hear what others think about the best practices for unit testing UI libraries. Let's work together to figure out the best approach. Thanks again for your feedback!

[endigo9740]

I am not a QA engineer

You could have fooled me my friend :D But sounds good, I agree with your points above. I'll noodle on things in the meantime and we can give this the proper attention it deserves in the near future when I'm not running around putting out fires.

Thanks again!

[amjadorfali]

@endigo9740
hahhaha I appreciate it 😅

Sounds awesome! Best of luck 🙏🏼

[endigo9740]

@amjadorfali again just letting you know I haven't forgot about you. This week has been all about ramping back up, collecting ourselves, and prepping for our future milestones for the project. We have a core team meeting scheduled tomorrow and testing will be one of the items discussed. I'll let you know where we fall on that asap.

[amjadorfali]

@endigo9740 thanks for the update, i really appreciate it. And no worries at all, you guys have been doing some amazing stuff! I'm looking forward to what comes next, and how i can contribute further in Skeleton🎉🚀

[endigo9740]

@amjadorfali I'm so sorry this has taken me so long to get back to. But I wanted to share where we've landed after talking with the team. Essentially it's what I stated before, the quality of test as presented is top notch, but we feel this might be overkill for us and much harder to maintain over time.

Going forward I'll go ahead and close this PR out, but we'll have it for reference going forward. Nothing will be lost. Then when an opportunity presents itself we'll devote a bit more time and attention to testing.

I know testing is a great practice, but there's also the common saying that "startups shouldn't use automated tests" and I think that's where we fall right now. I hope you understand. Hopefully over time things solidify a bit more and more in depth testing will be more practical.

I appreciate your effort and patience all the same!

[amjadorfali]

Hey @endigo9740 , I get that, it's understandable. Thanks for letting me know. I'll see where i can help elsewhere