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

Upgraded vue-cli dependencies and added unit testing capabilities to the vue sample #164

Closed
wants to merge 3 commits into from
Closed

Upgraded vue-cli dependencies and added unit testing capabilities to the vue sample #164

wants to merge 3 commits into from

Conversation

timbenniks
Copy link

  • Updated vue-cli dependencies to latest. Babel, jest, eslint/prettier work according to the latest specs now.
  • The new eslint/prettier configuration changed some components due to the length of the lines. No actual code was changed. Just line breaks and spaces are added. These are vue best practises.
  • Added JEST and @vue/test-utils to the codebase. One example test was added. Coverage threshold can be set in the package.json file.

Description

I noticed there are the basics of setting up unit tests for both the react and angular samples. The vue sample lacked this. When I added JEST and @vue/test-utils there were a bunch of issues with babel trying to transpile the spec files. As it turns out the vue-cli base dependencies were a little outdated. I decided to create a new vue-cli project with the same settings as the Vue sample app but I added unit tests on top. This resulted in working code transpilation and in updated prettier rules. All is now in line with the latest vue industry standards.

Motivation

I think these samples should have as little opinion to them as possible but should not lack basics for unit tests. I have added the "vue way" of how unit tests are done. No restrictions are given on how the tests are to be written. Coverage it completely configurable.

How Has This Been Tested?

  • Install by running npm i.
  • Run tests by running jss test:unit
  • See that the one test added passes
  • See that junit, lcov and cobertura coverage reports are outputted to coverage/

I coded the PR on macOS Mojave with node 10.13.0 and npm 6.7.0.
The changes in the templates of certain vue files are because of the newest prettier rules.
Regression test done by checking all components. They still work.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the Contributing guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.

* Updated vue-cli dependencies to latest. Babel, jest, prettier work according to the latest developments now.
* Added JEST and @vue/test-utils to the codebase. One example test was added. Coverage threshold can be set in the package.json file.
Copy link
Contributor

@aweber1 aweber1 left a comment

Choose a reason for hiding this comment

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

This looks good, thanks!

Unfortunately, the vue-apollo upgrade introduced a breaking change (https://github.com/Akryum/vue-apollo/releases) that is causing issues with SSR in Integrated Mode.

At present, I don't have the bandwidth to investigate and make the necessary changes to make vue-apollo happy. So there are a couple options:

  1. Feel free to tackle the changes (if you're comfortable in that Vue/GraphQL/SSR space) and update the PR.
  2. Roll back the vue-apollo update. Though I'm not entirely sure that beta.27 is compatible with Vue 2.6.x.

Along with the above, I have one request and one question:

  1. In package.json dependencies and devDependencies, please change all version ranges to be prefixed with ~ instead of ^. We have had too many suprises with dependencies being upgraded "silently" and inadvertently breaking the samples. Using ~ doesn't completely solve this, but it helps without having to pin versions.

  2. Question- this is really nit-picky, but can the coverageThreshold options in package.json be moved into jest.config.js? We try to keep "plugin" configuration out of package.json where possible to keep things consistent across samples. Naturally, there can/may be exceptions, so if it's more standard practice in Vue-land to have the coverageThreshold options in package.json or if there's solid reasoning to do so, then so be it.

Reverted back to vue 2.5.22 and vue-apollo beta 27 for compatibility reasons. Also removed opinionated coverage threshold settings
@timbenniks
Copy link
Author

@aweber1 thanks for your comment and the time spent on this.

  1. I have reverted the opinionated coverageThreshold options completely as JEST themselves use undefined as default setting. All configuration is now doable in jest.config.js.

  2. I have also added ~ instead of ^. The updating surprises should probably be resolved in another PR where we make JSS a vue-cli plugin. That way we should be more safe. When I have bandwidth I will have a look into that.

  3. I do not have the ability (for the moment) to go into connected mode so I have reverted vue to 2.5.22 and vue-apollo to beta 27.

Thanks

@sc-antonkulagin
Copy link
Contributor

Thank you for providing the changes. Unfortunately we have updated the dependencies without noticing this pull request. I`m closing this pr because the changes already been done.

@anastasiya29
Copy link
Contributor

@sc-antonkulagin - we upgraded the dependencies, but what about the unit test portion of the PR?
Adding sample unit tests has been a commonly request from the community.

@anastasiya29 anastasiya29 reopened this Jan 31, 2020
@anastasiya29 anastasiya29 requested review from sc-antonkulagin and sc-illiakovalenko and removed request for aweber1 January 31, 2020 21:44
@timbenniks
Copy link
Author

@anastasiya29 would it perhaps be better if I took the latest branch and add the tests? Merging this might be a challenge.

@sc-illiakovalenko
Copy link
Contributor

@timbenniks It would be great if you can leave only changes related to tests.

@timbenniks
Copy link
Author

@timbenniks It would be great if you can leave only changes related to tests.

Will do. I will however make a new PR as merging a year of new code into my fork might not be very easy.

@sc-illiakovalenko
Copy link
Contributor

@timbenniks I will close this PR, because you said that you will open another PR

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.

5 participants