Skip to content

Conversation

ceilfors
Copy link
Member

@ceilfors ceilfors commented Nov 5, 2017

What did you implement:

When trying to help on #266, I discovered that the plugin has 0% coverage at the moment for service.package.individually option in validate.js. This PR adds more unit tests for the missing coverage.

How did you implement it:

Unit tests are written.

How can we verify it:

Clone branch, run the test.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

Copy link
Member

@HyperBrain HyperBrain left a comment

Choose a reason for hiding this comment

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

Many thanks for adding the tests. Coverage did a big jump now!
If you could change the length check I commented, it's good to go ;-)

globSyncStub.callsFake(filename => [_.replace(filename, '*', 'js')]);
return expect(module.validate()).to.be.fulfilled
.then(() => {
expect(module.webpackConfig.length).to.equal(4);
Copy link
Member

Choose a reason for hiding this comment

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

Could be expressed as expect(module.webpackConfig).to.have.lengthOf(4); which will implicitly check for an array like structure.

@HyperBrain HyperBrain merged commit 922670a into serverless-heaven:master Nov 5, 2017
@ceilfors ceilfors deleted the missing-unit-test branch November 5, 2017 16:05
@HyperBrain
Copy link
Member

@ceilfors Just recognized that this breaks the unit tests on Windows (because of the fixed / in the expected paths). I will commit a fix into master to make them work again and use path.join to compose the paths.

@HyperBrain
Copy link
Member

Committed and fixed

@ceilfors
Copy link
Member Author

@HyperBrain Apologies! Maybe it's a good idea to add Windows to travis?

@HyperBrain
Copy link
Member

@ceilfors Yes, I planned to add AppVeyor as second build machine for the unit tests

jamesmbourne pushed a commit to jamesmbourne/serverless-webpack that referenced this pull request Oct 15, 2019
Add missing unit tests which cover package individually scenario.
jamesmbourne pushed a commit to jamesmbourne/serverless-webpack that referenced this pull request Oct 15, 2019
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.

2 participants