Unit tests in jest + minor cart store and hooks refactor #2414

lukeromanowicz · 2019-02-11T07:44:27Z

Related issues

Short description and why it's useful

Currently, existing tests (16 of them to be precise) written with mocha & karma are integration tests that might seem like a little bit chaotic. As VSF is growing fast, it's high time to use a more modern, comprehensive tool and start writing actual unit tests.

Upgrade Notes and Changelog

No upgrade steps required (100% backward compatibility)
I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

Contribution and currently important rules acceptance

I read and followed contribution rules
I read the TypeScript Action Plan and adjusted my PR according to it
I read about Vue Storefront Modules and I am aware that every new feature should be a module

Changes made in this PR:

removed karma tests and dependencies
added cart unit tests written in jest
moved e2e tests to from tests/ to test/ directory
added unit tests to Travis
Travis CI config revamp
minor cart refactoring

lukeromanowicz · 2019-02-12T13:35:42Z

@filrak All files in core/modules/cart except actions have full coverage, but I don't see much of a reason in testing actions. There are some areas to fix here and there in whole module including:

better docs for platformTotal and platformTotalSegments
support server_item_id in CART_UPD_ITEM mutation
supplement missing events (many functions have before-cart-x but don't have after-cart-x and vice versa) because, I suspect, rewriting everything to global actions is a no go because of backward compatibility(?)
a lot of events emitted by mutations contain current value but don't have a new one
almost no signs of hard typing
not all cart mutations modify its cartSavedAt
sometimes cartSavedAt is being updated, even if the primary change was not applied to state
definitely too much code in mutations not mentioning actions
alphabetical sorting would be nice considering how much code there is.
having parameters as an object is inconsistent, some function follow this rule, others don't
inconsistent parameter names in functions and emitted events
emitting 'application-level' events in unexpected places i.e. Vue.prototype.$bus.$emit('application-after-loaded') on load.

But I did the tests nonetheless. These issues are not super critical in opposite to what is happening to actions.ts alone:

dispatching actions belonging to the same namespace through rootStore
modifying store state directly from actions (not through mutations)
reading store state from other modules directly instead of through getters
hardcoded requests which could be moved to api
hardcoded notifications that could be grouped and stored separately
some logic could possibly be moved to helpers
100+ lines actions
nested promises instead of async functions
a lot of loggers that could be placed more wisely
let variables that often should be const instead
not to mention completely unreadable code

Because of that, I suggest refactoring this area first because the code will change significantly after refactoring and will have to be rewritten anyway.

@pkarw @patzick what do you think of it?

Also please CR

filrak · 2019-02-15T17:06:59Z

AWESOME job, thanks ;)
I guess the things that you mention are a good addition for the refactoring process we already discussed and will be discussed f2f next week (especially criticals that should be splitted into individual issues under same tag) ;). Regarding the tests I need more time to dig into it so I'll probably give my review on Monday.

Thanks again, huge step forward in quality

patzick · 2019-02-16T09:57:19Z

package.json

@@ -37,7 +37,7 @@
    "build:client": "cross-env NODE_ENV=production TS_NODE_PROJECT=\"tsconfig-build.json\" webpack --config ./core/build/webpack.prod.client.config.ts --mode production --progress --hide-modules",
    "build:server": "cross-env NODE_ENV=production TS_NODE_PROJECT=\"tsconfig-build.json\" webpack --config ./core/build/webpack.prod.server.config.ts --mode production --progress --hide-modules",
    "build": "rimraf dist && yarn build:client && yarn build:server && yarn build:sw",
-    "test:unit": "karma start ./tests/unit/karma.conf.js --single-run",
+    "test:unit": "yarn jest -c test/unit/jest.conf.js",


i think this should be without yarn, we're already calling yarn test:utils

any response to this?

I was certain that I fixed it last time. Will update in a minute

package.json

pkarw

Great job.

patzick · 2019-02-21T06:33:33Z

@lukeromanowicz is it still in WIP status?

lukeromanowicz · 2019-02-21T07:15:18Z

@patzick Yes, it is. I've been told to continue actions tests and clearing up dependencies is still todo.

lukeromanowicz · 2019-03-01T10:36:56Z

@filrak @patzick @pkarw it is done for now. Please review once again

pkarw · 2019-03-01T20:47:26Z

I think we can merge this in to 1.9rc - the final decision is up to @patzick and @filrak

patzick

Mostly looks good to me. It's nice that you updated travis workflow!
Just some minor improvements and conflict to resolve.

And yes - i think this should go to 1.9.0-rc.1 after review. @filrak WDYT?

patzick · 2019-03-03T14:33:00Z

core/modules/cart/store/actions.ts

      if ((Date.now() - context.state.cartServerTotalsAt) >= CART_TOTALS_INTERVAL_MS) {
-        context.state.cartServerPullAt = Date.now()
+        context.state.cartServerTotalsAt = Date.now()


this should be mutation

patzick · 2019-03-03T14:36:09Z

core/modules/cart/store/actions.ts

@@ -91,9 +92,9 @@ const actions: ActionTree<CartState, RootState> = {
    }
  },
  serverTotals (context, { forceClientState = false }) { // pull current cart FROM the server
-    if (rootStore.state.config.cart.synchronize_totals && !Vue.prototype.$isServer) {
+    if (rootStore.state.config.cart.synchronize_totals && !isServer) {


in future we'd like to get rid of config inside state because it is ~16kB of data which is barely needed on initial load. Please import config and apply suggestion

Suggested change

if (rootStore.state.config.cart.synchronize_totals && !isServer) {

if (config.cart.synchronize_totals && !isServer) {

patzick · 2019-03-03T14:36:46Z

core/modules/cart/store/actions.ts

@@ -110,7 +111,7 @@ const actions: ActionTree<CartState, RootState> = {
    }
  },
  serverCreate (context, { guestCart = false }) {
-    if (rootStore.state.config.cart.synchronize && !Vue.prototype.$isServer) {
+    if (rootStore.state.config.cart.synchronize && !isServer) {


Suggested change

if (rootStore.state.config.cart.synchronize && !isServer) {

if (config.cart.synchronize && !isServer) {

core/modules/cart/store/mutations.ts

patzick · 2019-03-03T14:44:22Z

package.json

@@ -37,7 +37,7 @@
    "build:client": "cross-env NODE_ENV=production TS_NODE_PROJECT=\"tsconfig-build.json\" webpack --config ./core/build/webpack.prod.client.config.ts --mode production --progress --hide-modules",
    "build:server": "cross-env NODE_ENV=production TS_NODE_PROJECT=\"tsconfig-build.json\" webpack --config ./core/build/webpack.prod.server.config.ts --mode production --progress --hide-modules",
    "build": "rimraf dist && yarn build:client && yarn build:server && yarn build:sw",
-    "test:unit": "karma start ./tests/unit/karma.conf.js --single-run",
+    "test:unit": "yarn jest -c test/unit/jest.conf.js",


any response to this?

patzick · 2019-03-03T14:45:38Z

test/unit/package.json

+{
+  "name": "@vue-storefront/unit-tests",
+  "private": true,
+  "version": "0.1.0"


this shouldn't be current storefront version?

patzick · 2019-03-05T12:33:02Z

@lukeromanowicz could you make this improvements today, so we could add this to upcoming RC version?

lukeromanowicz · 2019-03-06T10:50:31Z

@patzick thank you for your input. I didn't really plan to do any refactoring except necessary changes to make the code possible to test but in the end, I did the changes in accordance with your suggestions. Please re-review.

lukeromanowicz added 4 commits January 29, 2019 15:27

Initial jest unit tests

9a5a5c2

Unit tests utils, initial cart tests

170a8cc

Merge branch 'master' into feature/jest-tests

3b7a647

Fix tests after 1.7 -> 1.8 update

1d8eec8

lukeromanowicz mentioned this pull request Feb 11, 2019

[RFC] Unit tests revamp, Possible integration tests #2305

Closed

lukeromanowicz changed the title ~~WIP | Unit tests in jest~~ WIP | Unit tests in jest + minor cart store refactor Feb 11, 2019

lukeromanowicz changed the title ~~WIP | Unit tests in jest + minor cart store refactor~~ WIP | Unit tests in jest + minor cart store and hooks refactor Feb 11, 2019

lukeromanowicz added 3 commits February 11, 2019 11:14

Merge branch 'master' into feature/jest-tests

43663ef

Most of cart store tests, typescript building fix to ignore tests

9f4599a

cart tests code cleanup and old tests removal

fa4d9b6

lukeromanowicz changed the title ~~WIP | Unit tests in jest + minor cart store and hooks refactor~~ Unit tests in jest + minor cart store and hooks refactor Feb 12, 2019

lukeromanowicz changed the title ~~Unit tests in jest + minor cart store and hooks refactor~~ WIP | Unit tests in jest + minor cart store and hooks refactor Feb 12, 2019

pkarw requested review from patzick and filrak February 15, 2019 13:14

patzick reviewed Feb 16, 2019

View reviewed changes

pkarw approved these changes Feb 18, 2019

View reviewed changes

patzick changed the base branch from master to develop February 21, 2019 06:30

patzick added this to the 1.9 milestone Feb 21, 2019

lukeromanowicz added 4 commits February 28, 2019 10:23

additional actions tests

5246696

Merge branch 'develop' into feature/jest-tests

c44706a

Move e2e to test catalog

ecb03dc

fix tests after recent merge with develop

5536a16

lukeromanowicz force-pushed the feature/jest-tests branch 4 times, most recently from 9e3a86d to 5536a16 Compare March 1, 2019 09:48

lukeromanowicz force-pushed the feature/jest-tests branch 6 times, most recently from c4fbce7 to 83b18a4 Compare March 1, 2019 10:11

Add unit tests to travis

5f6a881

lukeromanowicz force-pushed the feature/jest-tests branch from 83b18a4 to 5f6a881 Compare March 1, 2019 10:23

Remove remaining karma tests

0f52926

lukeromanowicz changed the title ~~WIP | Unit tests in jest + minor cart store and hooks refactor~~ Unit tests in jest + minor cart store and hooks refactor Mar 1, 2019

lukeromanowicz requested review from patzick and pkarw March 1, 2019 10:37

pkarw approved these changes Mar 1, 2019

View reviewed changes

patzick requested changes Mar 3, 2019

View reviewed changes

filrak approved these changes Mar 5, 2019

View reviewed changes

lukeromanowicz added 2 commits March 6, 2019 07:47

Merge branch 'develop' into feature/jest-tests

be53730

Refactor config in actions, fix unit tests package version

87cbc05

lukeromanowicz requested a review from patzick March 6, 2019 10:48

patzick added 2 commits March 6, 2019 17:51

Merge branch 'develop' into feature/jest-tests

3b029f5

Update CHANGELOG.md

5fa6036

patzick approved these changes Mar 6, 2019

View reviewed changes

patzick merged commit df3e2fa into vuestorefront:develop Mar 6, 2019

patzick mentioned this pull request Mar 7, 2019

security update packages #2554

Merged

lukeromanowicz mentioned this pull request Mar 7, 2019

yarn tests:unit - invalid config file #2542

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests in jest + minor cart store and hooks refactor #2414

Unit tests in jest + minor cart store and hooks refactor #2414

lukeromanowicz commented Feb 11, 2019 •

edited

Loading

lukeromanowicz commented Feb 12, 2019 •

edited

Loading

filrak commented Feb 15, 2019 •

edited

Loading

patzick Feb 16, 2019

patzick Mar 3, 2019

lukeromanowicz Mar 6, 2019

pkarw left a comment

patzick commented Feb 21, 2019

lukeromanowicz commented Feb 21, 2019 •

edited

Loading

lukeromanowicz commented Mar 1, 2019

pkarw commented Mar 1, 2019

patzick left a comment

patzick Mar 3, 2019

patzick Mar 3, 2019

patzick Mar 3, 2019

patzick Mar 3, 2019

patzick Mar 3, 2019

patzick commented Mar 5, 2019

lukeromanowicz commented Mar 6, 2019 •

edited

Loading

	if (rootStore.state.config.cart.synchronize_totals && !isServer) {
	if (config.cart.synchronize_totals && !isServer) {

	if (rootStore.state.config.cart.synchronize && !isServer) {
	if (config.cart.synchronize && !isServer) {

Unit tests in jest + minor cart store and hooks refactor #2414

Unit tests in jest + minor cart store and hooks refactor #2414

Conversation

lukeromanowicz commented Feb 11, 2019 • edited Loading

Related issues

Short description and why it's useful

Upgrade Notes and Changelog

Contribution and currently important rules acceptance

Changes made in this PR:

lukeromanowicz commented Feb 12, 2019 • edited Loading

filrak commented Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkarw left a comment

Choose a reason for hiding this comment

patzick commented Feb 21, 2019

lukeromanowicz commented Feb 21, 2019 • edited Loading

lukeromanowicz commented Mar 1, 2019

pkarw commented Mar 1, 2019

patzick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patzick commented Mar 5, 2019

lukeromanowicz commented Mar 6, 2019 • edited Loading

lukeromanowicz commented Feb 11, 2019 •

edited

Loading

lukeromanowicz commented Feb 12, 2019 •

edited

Loading

filrak commented Feb 15, 2019 •

edited

Loading

lukeromanowicz commented Feb 21, 2019 •

edited

Loading

lukeromanowicz commented Mar 6, 2019 •

edited

Loading