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

Increases code coverage of UI #88

Draft
wants to merge 52 commits into
base: master
Choose a base branch
from
Draft

Increases code coverage of UI #88

wants to merge 52 commits into from

Conversation

cesaragv
Copy link
Collaborator

@cesaragv cesaragv commented May 8, 2020

This PR pushes the code coverage above 64%

@reelmatt
Copy link
Member

reelmatt commented May 8, 2020

Looking good! I see you also fixed some of the hook errors too, so close to passing!!

@reddigari
Copy link
Collaborator

Amazing job on the tests!!

I need to say though that switching from hooks to class methods quickly, just for the sake of testing, is dangerous. The reason they introduced hooks was to remove a lot of the bugginess and verbosity of class-based callbacks (definint state in constructors, binding every method, etc.). My fear is that changing this much at the last second will result in us releasing something that doesn't work.

That being said, the things I tried worked fine. Just a little hesitant. Thoughts?

@cesaragv
Copy link
Collaborator Author

cesaragv commented May 9, 2020

As @reddigari even though I made the changes, I am also hesitant. In the end, I found a way to make hook works with enzyme and mount, but would drastically reduce the code coverage (maybe just above 60%). I proposed to hold on on this PR, and mention the code coverage and testing results, but merge after the presentation on Thursday. If anything breaks, we would calmy have time to fix it. Otherwise, we might rush fixes that could potentially break other things

@cesaragv cesaragv marked this pull request as draft May 9, 2020 19:42
@cesaragv
Copy link
Collaborator Author

cesaragv commented May 9, 2020

Unless, of course, somebody has time to do a full manual regression

@cesaragv cesaragv mentioned this pull request May 12, 2020
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.

4 participants