Skip to content
This repository has been archived by the owner on Apr 30, 2024. It is now read-only.

Added Unit testing for all vue-vocabulary package #978

Merged
merged 22 commits into from
Oct 14, 2021
Merged

Added Unit testing for all vue-vocabulary package #978

merged 22 commits into from
Oct 14, 2021

Conversation

MuluhGodson
Copy link
Contributor

@MuluhGodson MuluhGodson commented Oct 13, 2021

Fixes

Closes #965 by @brylie
Closes #962 by @brylie

Description

Reinstated test for @vue-vocabulary package.

So, for contributing, each time a new core module is added, or an existing module is refactored, a test should be written for that module in the package and run using the unit's test.

To summarize, task completed include:

  • added a test unit for Vue-vocabulary
  • added test script for vue-vocabulary
  • added a general test script
  • updated README.md to reflect changes made

Technical details

Packages installed include:

    vue/cli-plugin-unit-jest": "~4.5.0
    jest": "^27.2.5
    vue/test-utils": "^1.0.3

Tests

Run the following commands to test each package individually

npm run test:vue-vocabulary

To run a general test, run this command

npm run test:unit

Screenshots

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the default branch of the repository (main or master).
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated 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.

@MuluhGodson MuluhGodson requested review from a team as code owners October 13, 2021 16:17
@MuluhGodson MuluhGodson requested review from nimishbongale and hugosolar and removed request for a team October 13, 2021 16:17
@brylie brylie requested review from brylie and dhruvkb and removed request for a team, nimishbongale and hugosolar October 13, 2021 16:19
Copy link
Contributor

@brylie brylie left a comment

Choose a reason for hiding this comment

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

Just a couple of minor lint issues.

packages/vue-vocabulary/tests/unit/Dummy.spec.js Outdated Show resolved Hide resolved
packages/vue-vocabulary/tests/unit/VCheckbox.spec.js Outdated Show resolved Hide resolved
packages/vue-vocabulary/tests/unit/jest.config.js Outdated Show resolved Hide resolved
scripts/test_unit.sh Outdated Show resolved Hide resolved
scripts/test_unit.sh Outdated Show resolved Hide resolved
scripts/test_unit.sh Outdated Show resolved Hide resolved
@brylie
Copy link
Contributor

brylie commented Oct 13, 2021

Overall, the *.scss tests don't strike me as very meaningful. They are essentially diffing files, which is done as part of code review already.

Copy link
Contributor

@brylie brylie left a comment

Choose a reason for hiding this comment

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

Let's move the project tests into a /tests/ directory in the project root, rather than in packages/<package-name>/

@MuluhGodson
Copy link
Contributor Author

to clarify, moving project tests into a /tests/ directory in the project root
will the structure now be <root_folder>\tests\<package_name>\tests ?

@MuluhGodson MuluhGodson requested a review from brylie October 14, 2021 07:24
Copy link
Contributor

@brylie brylie left a comment

Choose a reason for hiding this comment

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

Since this pull request intends to close #965, please ensure that all of the checklist items in #965 are complete here.

image

@MuluhGodson
Copy link
Contributor Author

Hello @brylie
A PR was merged yesterday that seperated the CI files. we no longer have a ci-fonts etc. we have just ci-build.yml, ci-lint.yml and ci-test.yml

@MuluhGodson
Copy link
Contributor Author

MuluhGodson commented Oct 14, 2021

also, you had recommended yesterday that since we have just a test for vue-vocabulary we should just place the test files in the root test folder, with no <package_name> sub-folders.

https://github.com/creativecommons/vocabulary/pull/978#issuecomment-942527511

@brylie
Copy link
Contributor

brylie commented Oct 14, 2021

@MuluhGodson, good points. The project structure has changed since the checklist was complete. I'm mainly concerned about deleting unused code and files. Are there any unused scripts or code remaining in the project related to this pull request?

tests/unit/Dummy.spec.js Outdated Show resolved Hide resolved
@MuluhGodson
Copy link
Contributor Author

  • restore testing code in a tests/ directory in the root of this project
  • ensure we have script(s) defined in package.json to invoke test suites npm run test:unit and npm run test:vue-vocabulary while both test will do the same things now I made a decision to leave them because the npm run test:unit is used by the CI pipeline to run a general test and when future test are included, this command will be relevant and it is a good idea to have the npm run test:vue-vocabulary solely for running test for the vue-vocabulary if test for the others are subsequently added.
  • update CI test runners to invoke restored test
    • ci-vue.yml A PR merge yesterday changed all test files to just ci-test.yml.
  • remove fonts test, since sass-jest is no longer maintained.
  • delete all unused scripts and code after refactoring

@MuluhGodson MuluhGodson requested a review from brylie October 14, 2021 07:44
@brylie
Copy link
Contributor

brylie commented Oct 14, 2021

I just browsed the code on your branch and it seems fairly clean. So, I acknowledge that I was mistaken with the comment above. Thanks also for revising the checklist 😄

I did notice there are two jest.config.js files. Can we consolidate those files so there is only one Jest configuration?

@brylie
Copy link
Contributor

brylie commented Oct 14, 2021

In any case, I've approved this pull request. I'll just wait for your response about the double Jest configs.

@brylie
Copy link
Contributor

brylie commented Oct 14, 2021

It's going to be great to have our CI scripts up and running again 😃 Thank you for your excellent work in this regard!

@MuluhGodson
Copy link
Contributor Author

@brylie oh yes, removing the jest.config.js found in the test folder has no effect so i think it's a good idea to delete it. I think i used it when i still ran the test using jest -c jest.config.js but now the test are run using vue-cli so it's not needed any further.
but the jest.config.js in the folder is necessary so that the jest test written for vue-vocabulary will be run with vue-cli

@MuluhGodson MuluhGodson reopened this Oct 14, 2021
@MuluhGodson
Copy link
Contributor Author

@brylie sorry I erroneously closed this with a comment. it's reopened now.

@brylie
Copy link
Contributor

brylie commented Oct 14, 2021

OK, I noticed you deleted the jest.config.js. Are there any config settings that should be merged into the remaining jest.config.js?

@brylie
Copy link
Contributor

brylie commented Oct 14, 2021

If not, I'll go ahead and merge this PR.

@MuluhGodson
Copy link
Contributor Author

No. i think the configs that were in the deleted jest.config.js are not useful in this context. It was for a previous setup I experimented with but was not happy with the results.

@brylie brylie merged commit f10cb7f into cc-archive:main Oct 14, 2021
@brylie brylie self-assigned this Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Restore project unit tests npm ERR! missing script: test:unit
2 participants