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

Added unit and e2e tests for the LicenseDetailsCard.vue component #132

Merged

Conversation

hemanth-hk
Copy link
Contributor

Fixes #118

Description

This PR adds unit and e2e tests for the LicenseDetailsCard.vue component

Checklist

  • 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.
  • 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
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.

@hemanth-hk hemanth-hk requested review from a team March 8, 2020 14:30
Copy link
Member

@akmadian akmadian left a comment

Choose a reason for hiding this comment

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

I noticed that a fair amount of tests were copied from your tests for LicenseCode, or tests from here were copied there. Generally, duplicating tests isn't great. It's not like the world is going to end, but it's also not a good habit to fall into. Generally, each component should handle tests specific to that component, and not its children components. Things specific to the children components should be left to them for testing.

Your test names tended to be very vague, please go back over your test names and be sure that it's not something like "Has the [xx] tag".

I got a lot of i18n warns, all of them are of this format:

console.warn node_modules/vue-i18n/dist/vue-i18n.common.js:35
    [vue-i18n] Cannot translate the value of keypath 'somepath-to-sometext'. Use the value of keypath as default.

Please address the comments I've made and we'll be ready for another review :) Good start!

tests/e2e/specs/LicenseDetailsCard.js Outdated Show resolved Hide resolved
tests/unit/specs/components/LicenseDetailsCard.spec.js Outdated Show resolved Hide resolved
tests/unit/specs/components/LicenseDetailsCard.spec.js Outdated Show resolved Hide resolved
tests/unit/specs/components/LicenseDetailsCard.spec.js Outdated Show resolved Hide resolved
@hemanth-hk
Copy link
Contributor Author

One of the main reasons for me copying the code was that the basic scaffolding of the files was the same. Thank you for letting me know about the best practices.

While I was watching some youtube tutorials I noticed that most of them had a test_utils.js file and kept some of the common functions in that file and imported it when needed. Can we have something like that cause I feel that's the DRY approach

@akmadian
Copy link
Member

@hemanth-hk Yes I think that would be a good idea for containing things like a mock store/ state and that sort of thing. Please open an issue in this repo explaining in detail the sorts of things you'd include in the utils file. This wouldn't be within the scope of this PR, but I'd be happy to get this implemented :)

Copy link
Member

@akmadian akmadian left a comment

Choose a reason for hiding this comment

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

e2e tests look good to me, but unit tests are (seemingly) all failing. Please try running unit tests on a fresh clone, and let me know if they are also failing for you. If they are, please diagnose and fix the issue(s) that are causing them to fail, if they're not, we can try to diagnose why :)

@hemanth-hk
Copy link
Contributor Author

Yes, all of the unit tests are failing, I haven't yet figured it out why this is happening. Either I will change something so that they work or once #130 is merged I will write the tests directly in the master where everything is working fine, Cause when I pushed the code everything was working fine and I don't find any errors in the code

I'll update you about this soon... Thank you for reviewing

@hemanth-hk
Copy link
Contributor Author

I finally figured it... :) Please review and tell the tests which I may have missed

Copy link
Member

@akmadian akmadian left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for working on this, you've improved a lot already since your first reivews :)

@akmadian akmadian merged commit 3d9cd4f into creativecommons:master Mar 15, 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.

Add unit and e2e tests for the LicenseDetailsCard component
2 participants