refactor(js-toolkit): make tests work in monorepo #133

wincent · 2020-10-06T18:05:09Z

This makes the tests (and therefore the build) of the two js-toolkit projects work in the monorepo. This was very fiddly and a bit like whack-a-mole, so you can see some of the things I had to try in the commit messages — not the cleanest history I've ever produced; sorry for that.

Some of the key things that are going on:

Many devDependencies hoisted and/or updated in order to get things working (I tried to touch as little as possible, to make this as minimal and safe as I could, but I ended up having to touch more than I would have liked).
Adjusting GitHub actions to do build and test instead of just test, and running the v2 js-toolkit tests as a separate job because they live outside of the main set of workspaces.
Tweaking .eslintignore and .prettierignore to reflect build artifacts that are there now that the build is working.
Because of package versions for TypeScript-related stuff, as well as the new structure here, had to tweak the tsconfig.json files a bit to avoid errors.

This fixes a bunch of spurious errors during test runs of the form: error TS2345: Argument of type 'import("/Users/greghurrell/code/liferay-frontend-projects/node_modules/@types/estraverse/node_modules/@types/estree/index").Program' is not assignable to parameter of type 'import("/Users/greghurrell/code/liferay-frontend-projects/node_modules/@types/estree/index").Program'. Types of property 'body' are incompatible. (ie. note the duplicate conflicting versions of the `@types/estree` package).

As I unbreak the build (in order to unbreak the tests), I find these generated files that need to be excluded in order to stop the linting from freaking right out.

Eliminates these (many instances): ts-jest[config] (WARN) There is a mismatch between your NodeJs version v10.16.0 and your TypeScript target esnext. This might lead to some unexpected errors when running tests with `ts-jest`. To fix this, you can check /~https://github.com/microsoft/TypeScript/wiki/Node-Target-Mapping Warning seems legit seeing as we use v10.16.0 in liferay-portal and we don't want to making "ESNext" output for that.

Gets rid of: warning Missing version in workspace at "projects/js-toolkit", ignoring.

Sorry for the terrible commit message, but manipulating so many things at once and trying different things to get: #133 ready, I got to the point where the lockfile had some unexpected changes in it and I had to blow it away and recreate it.

This is just what I did for ESLint in 47ac870; basically, as I unbreak the build, thus creating more files, I discover stuff that needs to be ignored.

So, unlike the other projects imported previously, you can't run the tests on the js-toolkit projects without doing a build first. So, we're going to run "build" for _all_ projects via "yarn workspaces run build". This means that we have to add no-op stubs in the projects that don't have them. Note: this isn't ideal and will probably be refactored very soon, because js-toolkit has a top-level "build" which builds everything, and then per-package "builds" that build alone. So, if we visit every workspace, we're actually building js-toolkit twice. This is ok, for now, only because it is a small project with 5 packages and so builds fast.

wincent · 2020-10-07T13:04:08Z

Just force-pushed with a bunch of wrangling on top. I think this one has at least a reasonable chance of working — 😂. Enough to take it out of draft mode at least. Sorry for the messy pull.

wincent · 2020-10-07T13:09:10Z

Woot! Of the three test jobs, the main one passes. Going to see if I can fix the Windows and v2 js-toolkit jobs now.

Example failure here[0]. As explained in the comment, the behavior of this code is sensitive to the environment that you run in, so I'm making the test looser to work both locally and in CI. In the monorepo, the `getRenderer()`[1] method is going to try to get "node-sass" first and it is going to find it because it is upstairs in the top-level "node_modules" folder. In CI, when we run the v2.x tests, we only do a yarn install inside "maintenance/projects/js-toolkit", because it doesn't participate in the top-level Yarn workspaces. So in CI, `getRenderer()` is going to try to get "node-sass", fail, and fallback to "sass", which is bundled more locally. As such, we need to loosen our assertion about what the renderer description looks like, as well as make the assertion about the expected output whitespace-agnostic, seeing as "node-sass" and "sass" make slightly different choices about whitespace. I figure this looseness is more future-proof and flexible than the alternatives, because it is more likely to keep the suite green even if we refactor and restructure the relationship between packages in the monorepo; and, at the end of the day, the `getRenderer()` is fundamentally dynamic in nature. [0]: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1220537713 [1]: /~https://github.com/liferay/liferay-frontend-projects/blob/47c938cc1c1a7a2837453eb96535fcec9a6492a3/maintenance/projects/js-toolkit/packages/liferay-npm-bundler-loader-sass-loader/src/index.js#L31-L58

Example failure: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1220537852 This might just be one of those classic "Windows hates globs" issues, so let's see if quoting fixes it.

My last attempt (parent commit) neither fixed Windows[0], nor allowed Linux[1] to keep working. It seems that `tsc` knows nothing about globs, so let's just call the command with the explicit, expanded list of packages. [0]: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1220654707 [1]: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1220537852

Error doesn't really say what is going wrong: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1220849394 So this is just a wild guess.

This one is a unique and beautiful snowflake anyway (because the v2 project lives off in its own world under "maintenance/", outside of the top-level set of Yarn workspaces), so there is no harm in letting it diverge a little bit from the other jobs.

Based on last failure: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1221023595 We can see that it isn't finding "yarn". So let's try going directly to "tsc" (of course, on Windows it is probably called "tsc.bat" or something", but we'll find that out when we hit CI).

wincent · 2020-10-07T15:01:02Z

Made some progress! Tests now actually run on Windows, with some failure that looks expected because we've never run them on Windows before:

FAIL projects/js-toolkit/packages/liferay-js-toolkit-core/src/file/__tests__/FilePath.test.ts
  ● in posix systems › toDotRelative works

    expect(received).toEqual(expected) // deep equality

    Expected: "./a/path"
    Received: "./a\\path"

      41 | 	it('toDotRelative works', () => {
      42 | 		expect(new FilePath('').toDotRelative().asNative).toEqual('.');
    > 43 | 		expect(new FilePath('a/path').toDotRelative().asNative).toEqual(
         | 		                                                        ^
      44 | 			'./a/path'
      45 | 		);
      46 | 

      at Object.<anonymous> (packages/liferay-js-toolkit-core/src/file/__tests__/FilePath.test.ts:43:59)

jbalsas · 2020-10-07T15:03:30Z

Made some progress! Tests now actually run on Windows, with some failure that looks expected because we've never run them on Windows before:

Getting closer!! 💪

As seen here: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1221124885 ``` FAIL projects/js-toolkit/packages/liferay-js-toolkit-core/src/file/__tests__/FilePath.test.ts ● in posix systems › toDotRelative works expect(received).toEqual(expected) // deep equality Expected: "./a/path" Received: "./a\\path" 41 | it('toDotRelative works', () => { 42 | expect(new FilePath('').toDotRelative().asNative).toEqual('.'); > 43 | expect(new FilePath('a/path').toDotRelative().asNative).toEqual( | ^ 44 | './a/path' 45 | ); 46 | at Object.<anonymous> (packages/liferay-js-toolkit-core/src/file/__tests__/FilePath.test.ts:43:59) ``` So, prior to this commit, we had tests that tried to exercise both POSIX and Windows environments by faking the environment. ie. you could be on your Linux machine and run the "Windows" examples and see them pass, but you obviously weren't testing real Windows and we were just setting `nativeIsPosix` to `false` (and vice a versa for the POSIX examples). Well, now that we run in a real Windows environment we see the above failure and it is because even though we set `nativeIsPosix` to `true`, it's not really true. We hit a `path.normalize()` call in the middle of that function that turns our POSIX separators back to Windows separators, as per the docs: > they are replaced by a single instance of the platform-specific > path segment separator ... Since Windows recognizes multiple path > separators, both separators will be replaced by instances of the > Windows preferred separator https://nodejs.org/api/path.html#path_path_normalize_path Obviously, `path.normalize()` is not affected by out little stub. So, the solution implemented in this commit is to remove the stubbing and instead only run the native platform tests. This means that when you run on Windows, you will run the Windows tests, and when you run on Linux or macOS, you will run the POSIX tests. And because CI runs both, we still cover all bases before anything gets merged.

wincent · 2020-10-07T15:37:32Z

Ok, so going to merge this even though it is a bit messy. Next steps should be cleaner than this!

This is still a bit rough (would like to follow up with a refactor commit), but it implements a basic `--interactive` mode which replaces this flow: npx @liferay/changelog-generator --dry-run # preview changes, decide on release type npx @liferay/changelog-generator --patch # actually generate changes git add -p # stage changes yarn version --patch # do release with this: npx @liferay/changelog-generator --interactive yarn version --patch It's a big ugly inside, but effectively we have a little state machine that goes through this cycle: 1. Run like `--dry-run`, and show a preview of what would be changed. 2. Prompt user for a version number (can be "major" etc or an explicit number). 3. Actually write out changes with the version number in effect. 4. Show `git diff` output and offer to `git add`. Test plan: Here's a demo of the basic "happy path" (obviously monochrome, but it's colorized in real life). I tested a few variants along the way and it seems to work for everything that I threw at it. $ node bin/liferay-changelog-generator.js --interactive ____________________________________ (_) ` | | | @liferay/changelog-generator | | ============================ | | | | Reporting for duty! | | | |__________________________________| (_)_________________________________) [--interactive] Using phony version changelog-generator/v0.0.0-placeholder.0 Fetching remote tags: run with --no-update-tags to skip [--interactive] Preview of changes 👀 ## [changelog-generator/v0.0.0-placeholder.0](/~https://github.com/liferay/liferay-frontend-projects/tree/changelog-generator/v0.0.0-placeholder.0) (2020-10-16) [Full changelog](changelog-generator/v1.5.0...changelog-generator/v0.0.0-placeholder.0) ### 🆕 Features - feat(monorepo): use liferay-npm-scripts for formatting ([\#146](#146)) - feat(changelog-generator): make --version optional when --dry-run is on ([\#116](#116)) ### 📖 Documentation - docs(changelog-generator): update project links ([\#94](#94)) ### 🤹‍♀️ Refactoring - refactor(js-toolkit): make tests work in monorepo ([\#133](#133)) [--interactive] Would write ./CHANGELOG.md 7413 bytes ✨ 1. major 2. minor 3. patch 4. premajor 5. preminor 6. prepatch 7. prerelease Please choose a version from the list, or provide one in full (or enter to abort): 2 Wrote ./CHANGELOG.md 7371 bytes ✨ [--interactive] git-diff preview 👀 diff --git a/projects/npm-tools/packages/changelog-generator/CHANGELOG.md b/projects/npm-tools/packages/changelog-generator/CHANGELOG.md index fdf781a4..2ccfe5fa 100644 --- a/projects/npm-tools/packages/changelog-generator/CHANGELOG.md +++ b/projects/npm-tools/packages/changelog-generator/CHANGELOG.md @@ -1,3 +1,20 @@ +## [changelog-generator/v1.6.0](/~https://github.com/liferay/liferay-frontend-projects/tree/changelog-generator/v1.6.0) (2020-10-16) + +[Full changelog](changelog-generator/v1.5.0...changelog-generator/v1.6.0) + +### 🆕 Features + +- feat(monorepo): use liferay-npm-scripts for formatting ([\#146](#146)) +- feat(changelog-generator): make --version optional when --dry-run is on ([\#116](#116)) + +### 📖 Documentation + +- docs(changelog-generator): update project links ([\#94](#94)) + +### 🤹‍♀️ Refactoring + +- refactor(js-toolkit): make tests work in monorepo ([\#133](#133)) + ## [changelog-generator/v1.5.0](/~https://github.com/liferay/liferay-frontend-projects/tree/changelog-generator/v1.5.0) (2020-09-29) [Full changelog](changelog-generator/v1.4.1...changelog-generator/v1.5.0) Would you like to stage these changes? (y/n) y $ git status On branch wincent/version-shortcuts Your branch is ahead of 'master' by 6 commits. (use "git push" to publish your local commits) Changes to be committed: (use "git restore --staged <file>..." to unstage) modified: CHANGELOG.md

github-actions bot added 2.x 3.x js-toolkit labels Oct 6, 2020

wincent added 9 commits October 7, 2020 10:15

refactor(js-toolkit): make tests work in monorepo

2f33c91

chore(js-toolkit): add more paths to .eslintignore

47ac870

As I unbreak the build (in order to unbreak the tests), I find these generated files that need to be excluded in order to stop the linting from freaking right out.

chore(js-toolkit): add dummy version number to silence Yarn warning

eb0e696

Gets rid of: warning Missing version in workspace at "projects/js-toolkit", ignoring.

chore: freshen yarn.lock

1a853e1

Sorry for the terrible commit message, but manipulating so many things at once and trying different things to get: #133 ready, I got to the point where the lockfile had some unexpected changes in it and I had to blow it away and recreate it.

chore(js-toolkit): add more paths to .prettierignore

67f548a

This is just what I did for ESLint in 47ac870; basically, as I unbreak the build, thus creating more files, I discover stuff that needs to be ignored.

fix(js-toolkit): wrangle tests and build so that they work in monorepo

039c76b

wincent force-pushed the wincent/make-tests-pass branch from 3cad687 to 039c76b Compare October 7, 2020 13:03

github-actions bot added changelog-generator eslint-plugin jest-junit-reporter js-insights js-publish npm-bundler-preset-liferay-dev npm-scripts npm-tools labels Oct 7, 2020

wincent marked this pull request as ready for review October 7, 2020 13:04

wincent added 4 commits October 7, 2020 15:18

test(js-toolkit): attempt to make tests pass in Windows on CI

2b4b6c8

Example failure: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1220537852 This might just be one of those classic "Windows hates globs" issues, so let's see if quoting fixes it.

test(js-toolkit): make another attempt at getting Windows passing CI

8437838

Error doesn't really say what is going wrong: /~https://github.com/liferay/liferay-frontend-projects/pull/133/checks?check_run_id=1220849394 So this is just a wild guess.

wincent force-pushed the wincent/make-tests-pass branch from c93aa4d to 8437838 Compare October 7, 2020 14:12

feat(js-toolkit): add logging as debugging aid for Windows CI issues

47c73a9

wincent force-pushed the wincent/make-tests-pass branch from 1dd4d2e to 47c73a9 Compare October 7, 2020 14:35

wincent added 2 commits October 7, 2020 16:45

test(js-toolkit): attempt to find tsc with ".cmd" extension on Windows

3556464

wincent merged commit 7cfa7c6 into master Oct 7, 2020

wincent deleted the wincent/make-tests-pass branch October 7, 2020 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(js-toolkit): make tests work in monorepo #133

refactor(js-toolkit): make tests work in monorepo #133

wincent commented Oct 6, 2020 •

edited

Loading

wincent commented Oct 7, 2020

wincent commented Oct 7, 2020

wincent commented Oct 7, 2020

jbalsas commented Oct 7, 2020

wincent commented Oct 7, 2020

refactor(js-toolkit): make tests work in monorepo #133

refactor(js-toolkit): make tests work in monorepo #133

Conversation

wincent commented Oct 6, 2020 • edited Loading

wincent commented Oct 7, 2020

wincent commented Oct 7, 2020

wincent commented Oct 7, 2020

jbalsas commented Oct 7, 2020

wincent commented Oct 7, 2020

wincent commented Oct 6, 2020 •

edited

Loading