Supports NPM 8 and convert tests from Mocha to Jest #1084

moroine · 2022-03-04T21:39:23Z

What did you implement:

Closes #1083 #805 #374

How did you implement it:

NPM 8 automatically installs peerDependencies causing a lot of WARNINGS: Could not determine version of module XXXX. In this case, we can use directly package-lock.json which has the same shape as the npm ls result.
Reduce the number of external modules included by using webpack.optimization.usedExports

How can we verify it:

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

j0k3r

Could you add a specific test scenario where NPM 8 is used (or when NPM < 8 is used) like we did for Serverless?

README.md

moroine · 2022-03-09T22:10:39Z

Could you add a specific test scenario where NPM 8 is used (or when NPM < 8 is used) like we did for Serverless?

@j0k3r I've added tests, unfortunately, I'm not very used to the test framework used (I feel more comfortable with Jest). Also I think it would be nice to have end-to-end testing test cases.

vicary · 2022-03-10T04:26:45Z

@moroine For e2e tests you may take a look at the testing tool runServerless used by core. I have written one test at serverless/serverless#10177, it's not as sophisticated as a complete framework like jest, but it's doing a good job emulating the lifecycle events.

moroine · 2022-03-10T08:55:05Z

@vicary in order to install @serverless/test I need to upgrade to mocha@9 but mocha@9 drop supports of NodeJs 10.

As NodeJs 10 is deprecated, do we want to drop the support? I can just create some test projects within tests and try using serverless package, but the tests would be way slower 🤔

j0k3r · 2022-03-10T08:57:35Z

I tried to add some integration test but didn't succeed as of now: #1019

I used the previous release of serverless/test which sur mocha@8

moroine · 2022-03-10T13:47:26Z

@j0k3r @vicary I managed to add end-to-end tests. I think we can use example folder as fixtures files 🤔

We can see the difference when using the package-lock.json. We could in future iteration try to use lock version when possible to prevent unexpected side-effects

moroine · 2022-03-13T20:28:01Z

examples/include-external-npm-packages-lock-file/_setup.js

@@ -0,0 +1,21 @@
+'use strict';


In order to be able to run the fixture in tests, we need to tweak some files (especially because they are copied over a temporary directory)

moroine · 2022-03-13T20:31:12Z

tests/e2e/e2eUtils.js

+async function runServerless(options) {
+  const runServerlessOptions = {
+    command: 'package'
+  };
+
+  const servicePath = await setupFixture(options.fixture);
+
+  runServerlessOptions.cwd = servicePath;
+  const SERVERLESS_DIR = path.join(servicePath, 'node_modules', 'serverless');
+  try {
+    const result = await originalRunServerless(SERVERLESS_DIR, runServerlessOptions);
+    result.servicePath = servicePath;
+    return result;
+  } catch (error) {
+    error.servicePath = servicePath;
+    throw error;
+  }
+}


This is very similar to @serverless/test but the issue with the original one is they:

run npm install

execute _setup

This cause 2 issues:

Cannot fix relative paths

Cannot use yarn

moroine · 2022-03-13T20:46:14Z

@j0k3r @vicary I've fixed (I think) the end-to-end tests. @serverless/test doesn't fullfill our use case. Might be good to PR them but I don't have time to do it as of now.

moroine · 2022-03-17T22:56:58Z

@j0k3r @vicary after a lot of tries & failures I managed to make the tests finally work but at a cost of huge changes...

This PR adds end-to-end tests, but I had a lot of issues due to mocking modules in tests (fs, glob, ...) but for e2e we won't want to mock. It was working when running alone but not with the entire suite test. To fix this I had to remove mocha and use jest which has better module mock support in isolation...

Not sure if you're ok with this PR changing so much at once 🤔

vicary · 2022-03-18T09:44:00Z

@moroine Huge thanks for the effort!

I personally don't mind huge changes, it just takes time to review, largely making sure existing tests are correctly ported. I'll take some time in the weekend for this.

Note: We want to make sure coverage is not dropped, we temporarily disabled the drop protection for the v3 PR but for most cases it is still enforced.

j0k3r

Good job, really!

I'm not against switch from Mocha to Jest, I use jest on a daily basis.

__mocks__ folder should be added to .npmignore

jest.config.js

lib/packExternalModules.js

lib/runPluginSupport.js

tests/e2e/e2e.test.js

package.json

j0k3r · 2022-03-24T10:57:23Z

We merged some PRs today.
Can you rebase against the master to fix conflicts?

Co-authored-by: Jérémy Benoist <j0k3r@users.noreply.github.com>

moroine · 2022-03-24T13:08:07Z

@j0k3r I've rebase the changes

j0k3r · 2022-03-24T13:32:13Z

Closed/reopen to re-trigger GitHub Actions

vicary

Sorry I've got a busy weekend, just a few tweaks and we are good to go.

README.md

tests/packagers/index.test.js

…npm-8

j0k3r · 2022-03-29T03:49:46Z

Why did you link the issue #374?

moroine · 2022-03-29T05:54:02Z

Why did you link the issue #374?

Because with isUsedExports we only includes externals present in the final build. Tree-shaking will removed unused dependencies.

moroine · 2022-03-29T07:01:05Z

Are we good to merge?

moroine changed the title ~~Feature/npm 8~~ Supports NPM 8 Mar 4, 2022

j0k3r reviewed Mar 8, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

moroine requested a review from j0k3r March 9, 2022 22:08

moroine marked this pull request as ready for review March 9, 2022 22:13

moroine commented Mar 13, 2022

View reviewed changes

moroine force-pushed the feature/npm-8 branch from 6361ca7 to f52bb30 Compare March 15, 2022 19:23

j0k3r reviewed Mar 18, 2022

View reviewed changes

jest.config.js Show resolved Hide resolved

jest.config.js Outdated Show resolved Hide resolved

lib/packExternalModules.js Outdated Show resolved Hide resolved

lib/runPluginSupport.js Outdated Show resolved Hide resolved

tests/e2e/e2e.test.js Outdated Show resolved Hide resolved

moroine requested a review from j0k3r March 18, 2022 11:03

j0k3r reviewed Mar 18, 2022

View reviewed changes

package.json Show resolved Hide resolved

moroine requested a review from j0k3r March 21, 2022 08:13

j0k3r requested a review from vicary March 21, 2022 19:37

j0k3r added this to the 5.7.0 milestone Mar 24, 2022

j0k3r requested a review from miguel-a-calles-mba March 24, 2022 10:57

moroine and others added 7 commits March 24, 2022 13:38

feat: support NPM 8 peerDependencies

3f864d9

feat: include only usedExports

f379b59

chore: update readme

2762e31

Co-authored-by: Jérémy Benoist <j0k3r@users.noreply.github.com>

chore: add tests

7e1b84d

chore: fix eslint

db51861

fix: windows path delimiter

8f70f26

fix: path sepeartor

ee4ecfc

moroine added 10 commits March 24, 2022 13:43

chore: fix window tests

708021a

chore: fix window tests

bc13070

chore: fix window tests

d35e19b

chore: fix window tests

fb7dfb1

chore: fix window tests

a1b37f3

chore: fix window tests

9d895ea

chore: address j0k3r feedbacks

442957b

chore: remove indentation

16dd6ff

chore: fix prettier

8dec6d1

fix: merge conflicts

a6e58cf

moroine force-pushed the feature/npm-8 branch from 0466c8c to a6e58cf Compare March 24, 2022 13:07

j0k3r closed this Mar 24, 2022

j0k3r reopened this Mar 24, 2022

vicary requested changes Mar 25, 2022

View reviewed changes

README.md Show resolved Hide resolved

tests/packagers/index.test.js Outdated Show resolved Hide resolved

moroine added 2 commits March 28, 2022 21:15

chore: address PR comments

fcdcd94

Merge remote-tracking branch 'serverless-heaven/master' into feature/…

d71cd49

…npm-8

moroine requested a review from vicary March 28, 2022 19:18

miguel-a-calles-mba approved these changes Mar 28, 2022

View reviewed changes

j0k3r approved these changes Mar 29, 2022

View reviewed changes

j0k3r linked an issue Mar 29, 2022 that may be closed by this pull request

Serverless: WARNING: Could not determine version of module xxxxx #805

Closed

j0k3r changed the title ~~Supports NPM 8~~ Supports NPM 8 and convert tests from Mocha to Jest Mar 29, 2022

vicary approved these changes Mar 29, 2022

View reviewed changes

j0k3r linked an issue Mar 29, 2022 that may be closed by this pull request

Optimize Package #374

Closed

j0k3r merged commit 89c629e into serverless-heaven:master Mar 29, 2022

moroine deleted the feature/npm-8 branch March 29, 2022 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supports NPM 8 and convert tests from Mocha to Jest #1084

Supports NPM 8 and convert tests from Mocha to Jest #1084

moroine commented Mar 4, 2022 •

edited

j0k3r left a comment

moroine commented Mar 9, 2022

vicary commented Mar 10, 2022

moroine commented Mar 10, 2022

j0k3r commented Mar 10, 2022

moroine commented Mar 10, 2022

moroine Mar 13, 2022

moroine Mar 13, 2022

moroine commented Mar 13, 2022

moroine commented Mar 17, 2022

vicary commented Mar 18, 2022

j0k3r left a comment

j0k3r commented Mar 24, 2022

moroine commented Mar 24, 2022

j0k3r commented Mar 24, 2022

vicary left a comment

j0k3r commented Mar 29, 2022

moroine commented Mar 29, 2022

moroine commented Mar 29, 2022

Supports NPM 8 and convert tests from Mocha to Jest #1084

Supports NPM 8 and convert tests from Mocha to Jest #1084

Conversation

moroine commented Mar 4, 2022 • edited

What did you implement:

How did you implement it:

How can we verify it:

Todos:

j0k3r left a comment

Choose a reason for hiding this comment

moroine commented Mar 9, 2022

vicary commented Mar 10, 2022

moroine commented Mar 10, 2022

j0k3r commented Mar 10, 2022

moroine commented Mar 10, 2022

moroine Mar 13, 2022

Choose a reason for hiding this comment

moroine Mar 13, 2022

Choose a reason for hiding this comment

moroine commented Mar 13, 2022

moroine commented Mar 17, 2022

vicary commented Mar 18, 2022

j0k3r left a comment

Choose a reason for hiding this comment

j0k3r commented Mar 24, 2022

moroine commented Mar 24, 2022

j0k3r commented Mar 24, 2022

vicary left a comment

Choose a reason for hiding this comment

j0k3r commented Mar 29, 2022

moroine commented Mar 29, 2022

moroine commented Mar 29, 2022

moroine commented Mar 4, 2022 •

edited