Feature/unit test #2

GianlucaGuarini · 2018-08-17T14:18:43Z

This is a big PR.
in a nutshell it improves the following things:

Add the Circle-ci integration
Add the Codeclimate Integration
Add the Coverall integration
Add unit tests for everything
Clean up the package dependencies
Add the initial interfaces folder

@nirazul @faebeee Please let me know if you have questions

GianlucaGuarini · 2018-08-17T14:20:21Z

The coverace fails because actually I wasn't able to test completely the scroll-lock-helper mixin since the scroll vuex module is still missing in this repo

nirazul

Thanks for the effort, it's a very nice first step for a core component library!

A few questions:

Do we really need to check in the dist file dist/index.esm.js? Is there a reason?

The rest of the questions is inlined, feel free to ask, any time!

nirazul · 2018-08-22T06:47:55Z

src/components/modal/modal.spec.js

+
+import { createLocalVue, shallowMount } from '@vue/test-utils';
+import { expect } from 'chai';
+import { Modal } from '../../../';


This looks sort of an error to me? Is it like omitting index.js?

the node require algo will import always what we define in the main key of the package.json. Now if today we use main: "dist/index.cjs.js" and tomorrow main: "my-fancy-lib.js" we can avoid to rewrite all the paths in our unit test.

Sidenote: it's impossible to unit test vue components in node without bundling them

nirazul · 2018-08-22T06:48:48Z

src/components/modal/modal.spec.js

@@ -0,0 +1,67 @@
+/* eslint-disable no-unused-expressions, max-nested-callbacks */


What was the unused expression?

if you use chaijs expectations you might want to write:

expect('Rouven').to.be.ok expect(['Gian', 'Luc', 'a'].to.have.a.length

In this case eslint complains because these chains are not called explicitly :(

nirazul · 2018-08-22T06:53:46Z

src/mixins/bem/bem.spec.js

+            const vm = new localVue(getDummyComponentProps(bemMixin(ROOT_CLASS)));
+
+            expect(vm.bemFacets).to.be.an('array');
+            expect(vm.bemFacets).to.include(`${ ROOT_CLASS }${ DEFAULT_OPTIONS.bemModifierMarker }${ DEFAULT_OPTIONS.defaultFacet }`);


I'd advise to expressively write down the expected result and avoid any kind of concat or logic within a test:
expect(vm.bemFacets).to.include('root__base');

This makes it much more readable and direct and avoids having subtle bugs within our unit tests.

It makes sense

nirazul · 2018-08-22T06:55:17Z

src/modules/overlay/actions.js

@@ -0,0 +1,60 @@
+import Vue from 'vue';
+import { CLOSE_OVERLAY, MOUNT_OVERLAY, OPEN_OVERLAY, PREPARE_CLOSE_OVERLAY, UNMOUNT_OVERLAY } from '../mutation-types';


I guess, you wanted to write it expressively, rather than import * as types from '../mutation-types'? Right?

yes it's just my personal preference

nirazul · 2018-08-22T06:56:21Z

src/modules/overlay/actions.js

+ * Trigger close action for an overlay
+ * @param {Object} context - A vuex action context
+ * @param {Object} payload - A vuex action payload
+ * @return {Promise} promise resolved once the last overlay commit will be dispatched


I don't understand what this should mean, what is the "last overlay commit"?
This text should describe what I get when I invoke the function

honestly I am still not really happy with this reducer. It dispatches too many asynchronous commits. I guess we will need to refactor it anyways

nirazul · 2018-08-22T07:01:36Z

src/modules/overlay/module.spec.js

@@ -0,0 +1,263 @@
+/* eslint-disable max-nested-callbacks */


You could disable max-nested-callbacks in the eslint config for all *.spec* files with the oerrides option:
https://eslint.org/docs/user-guide/configuring#disabling-rules-only-for-a-group-of-files

ah wow cool I didn't know it

GianlucaGuarini added 12 commits August 16, 2018 14:50

[unit-test] Add initial unit test setup

6947e74

[unit-test] Complete the locking scroll unit test

3edf35a

[unit-test] Complete overlay module unit test

98abbe6

[unit-test] Setup circle ci and coveralls

3d91ae0

[unit-test] Remove comments from the circle ci config

ca06152

[unit-test] Store artifacts

b098fa1

[unit-test] Update circle ci badge url

6f6b076

[unit-test] Add tests also for the mutations

955048b

[unit-test] Add more badges

4c81f7a

[unit-test] Add the codeclimate settings file

eda8c79

[unit-test] Add tests for vue components and mixins

1e8131f

[unit-test] Add initial unit test for the scroll-lock helper

445d47f

nirazul reviewed Aug 22, 2018

View reviewed changes

GianlucaGuarini added 3 commits August 22, 2018 11:57

[unit-test] Use the eslint overrides

e0b7ca5

[unit-test] Prefer static strings for the bem mixin unit test

b338630

[unit-test] Remove the generated files

e1a83ed

GianlucaGuarini merged commit e6c630a into master Aug 22, 2018

GianlucaGuarini deleted the feature/unit-test branch August 22, 2018 11:54

		@@ -0,0 +1,67 @@
		/* eslint-disable no-unused-expressions, max-nested-callbacks */

		@@ -0,0 +1,60 @@
		import Vue from 'vue';
		import { CLOSE_OVERLAY, MOUNT_OVERLAY, OPEN_OVERLAY, PREPARE_CLOSE_OVERLAY, UNMOUNT_OVERLAY } from '../mutation-types';

Feature/unit test #2

Feature/unit test #2

Uh oh!

Conversation

GianlucaGuarini commented Aug 17, 2018

Uh oh!

GianlucaGuarini commented Aug 17, 2018

Uh oh!

nirazul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants