Adds unit and e2e tests for CopyrightWaiverStep Component

[JackieBinya]

Fixes

Related #133 by @akmadian

Description

• Adds unit tests
• Adds e2e tests

Technical details

Tests

Screenshots

Checklist

• [x ] My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the master branch of the repository.
• My commit messages follow best practices.
• [x ] My code follows the established code style of the repository.
• [x ] I added tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no
  visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[obulat]

Thank you for contribution, @JackieBinya , it looks great!
Could you please run the linter on the file? After that, there are only two minor issues that I will comment on on the corresponding lines.

[JackieBinya]

@obulat I have cleared the errors as advised in the review.
Well, I have an issue with the test coverage when I run npm run test, the results are in the screenshot below. Somehow lines 57, 58, 70,71 have not been tested in the unit tests in CopyrightWaiverStep.spec.js

After going through the logic what I realized was that the logic makes it seem like there are checkboxes where the user has an option to check not to agree OR not confirmed. But in reality, that's not the case the user can only check agree or confirm.
So I am in a situation where I'm stuck I don't have a checkbox I can check and trigger events on the else if statements on lines 57,58,70,71

[obulat]

Coverage shows that 4 lines are not covered. They have to do with the state when both checkboxes are checked, and the user 'deselects' one of them. Once you add tests for them, I will approve this PR.

[obulat]

I guess you have already understood the answer to your question, but I will still write it just in case:

The user can check both checkboxes at first, but then go back and click on one of them again. If this happens, the CopyrightWaiverStep should not let the user proceed. So, if you set both props to true, and then click on them (setChecked(false)) and test for the emitted event, you will cover the remaining lines.

[JackieBinya]

Hi, @obulat I have added the tests. But I had a problem with my Github account for some reason it was not showing all my pushed commits, hence the reason, I pushed the same commit twice.
Well in my implementation I used checkbox.trigger('change') instead of setChecked(false) Only because my github was not updating hence I was not able to see your recommendations. But despite that, I have tested the emitted events in the if-else block line 58&71. Coverage still says line 57 and 70 are yet to be checked. But overall coverage has improved.
Please check out the screenshot below.

In the meantime, I will try to write the e2e tests as I await your review.

[obulat]

Hi, @obulat I have added the tests. But I had a problem with my Github account for some reason it was not showing all my pushed commits, hence the reason, I pushed the same commit twice.

Actually, github was down about 7-8 hours ago, I think that's the reason for some problems you had. Don't worry!

Well in my implementation I used checkbox.trigger('change') instead of setChecked(false) Only because my github was not updating hence I was not able to see your recommendations. But despite that, I have tested the emitted events in the if-else block line 58&71. Coverage still says line 57 and 70 are yet to be checked. But overall coverage has improved.
Please check out the screenshot below.

I tried to figure it out, but I don't quite understand how that line can be untested, when we actually do get inside that condition, and the line after 'if' is executed.
However, I found a solution for how to cover those lines. In general, it is better to treat the code as a black box when you are testing it. So, it is better to approach it as a user clicking on a checkbox rather than a coder who knows how to set the props behind that checkbox. In this case, you set the agreed and confirmed properties in the beginning by using setData. Instead, you can set them to true by clicking both checkboxes. Then, to actually test 'unchecking', you can click on one of the checkboxes again. This way, those two lines get covered.

[JackieBinya]

Hi, @obulat I have added the tests. But I had a problem with my Github account for some reason it was not showing all my pushed commits, hence the reason, I pushed the same commit twice.

Actually, github was down about 7-8 hours ago, I think that's the reason for some problems you had. Don't worry!

Well in my implementation I used checkbox.trigger('change') instead of setChecked(false) Only because my github was not updating hence I was not able to see your recommendations. But despite that, I have tested the emitted events in the if-else block line 58&71. Coverage still says line 57 and 70 are yet to be checked. But overall coverage has improved.
Please check out the screenshot below.

I tried to figure it out, but I don't quite understand how that line can be untested, when we actually do get inside that condition, and the line after 'if' is executed.
However, I found a solution for how to cover those lines. In general, it is better to treat the code as a black box when you are testing it. So, it is better to approach it as a user clicking on a checkbox rather than a coder who knows how to set the props behind that checkbox. In this case, you set the agreed and confirmed properties in the beginning by using setData. Instead, you can set them to true by clicking both checkboxes. Then, to actually test 'unchecking', you can click on one of the checkboxes again. This way, those two lines get covered.

Thank you so much for that pointer @obulat. It will forever change my approach to writing unit tests. Finally, I had my 'Aha' moment. I was familiar with the approach of treating logic/code as Blackbox but today I finally understood. Coverage is now at 100%

[obulat]

Thank you so much for that pointer @obulat. It will forever change my approach to writing unit tests. Finally, I had my 'Aha' moment. I was familiar with the approach of treating logic/code as Blackbox but today I finally understood.

Great! To be honest, I got this approach after reading the article from vuejsdevelopers.com on "Knowing what to test".