Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Jun 23, 2020

Extracted from #5011 to land the changes in isolation from the actual enablement of parallel testing.

The first commit is cherry-picked 20da28e, then I added more commits to clean up the code more to my liking.

My intention is to further clean up the commit history once we agree on the implementation direction & details.

To verify parallel tests, one can run the test suite via npm test -- --parallel. Some of the tests are failing in parallel mode on a timeout, I think that's something to investigate as part of enabling parallel mode (outside of this PR).

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@bajtos bajtos added the Internal Tooling Issues related to our tooling and monorepo infrastructore label Jun 23, 2020
@bajtos bajtos requested a review from raymondfeng June 23, 2020 15:21
@bajtos bajtos requested review from agnes512 and emonddr as code owners June 23, 2020 15:21
@bajtos bajtos self-assigned this Jun 23, 2020
@bajtos bajtos mentioned this pull request Jun 23, 2020
14 tasks
@raymondfeng
Copy link
Contributor

@bajtos Please fix code formatting.

@raymondfeng
Copy link
Contributor

lodash is a much smaller dependency than other ones we already have for @loopback/build, such as typescript, nyc, mocha, prettier.

@bajtos
Copy link
Member Author

bajtos commented Jun 25, 2020

lodash is a much smaller dependency than other ones we already have for @loopback/build, such as typescript, nyc, mocha, prettier.

Fair enough.

Can we use https://lodash.com/docs/4.17.15#mergeWith instead?

I am not comfortable with using mergeWith's deep merge algorithm, I am concerned that non-scalar config options (like require) may require specific handling and the default deep-merge implementation may create unwanted outcomes.

Let's use assignWith for the first version, learn more about real-world usage and then improve the merging algorithm as needed.

@bajtos bajtos force-pushed the feat/parallel-snaphots branch 3 times, most recently from aba0a64 to f7d26cf Compare June 25, 2020 09:11
@bajtos
Copy link
Member Author

bajtos commented Jun 25, 2020

I rebased the patch on top of the latest master, addressed review comments and made few more improvements/cleanups:

  • fix linter issues

  • simplify mergeMochaConfigs using lodash

  • clean up packages/cli/.mocharc.js

  • improve comment in root .mocharc.js

  • cleanup snapshot matcher:

    1. Rename "state" to "snapshotMatcher" ("matcher"). I find "state" as too ambiguous, let's think about the new design as having one SnapshotMatcher instance for each snapshot directory. It is possible to further refactor the code from current functional-programming style into OOP by introducing SnapshotMatcher class, but I feel such change is not needed now.
    2. Move definition of root hook functions out of the hooks object, to make it easier to build a high-level understanding of the hooks without having to skip through implementation details.
    3. Move "currentTest" from individual states (matcher) into a single global variable, there is no need to duplicate it in every snapshot matcher instance.

@raymondfeng I consider the PR as ready for final review, PTAL.

@bajtos
Copy link
Member Author

bajtos commented Jun 25, 2020

(I'll clean up the commit history before merging.)

bajtos and others added 2 commits June 26, 2020 07:58
Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
…sting

- For each snapshotDir, share the same instance of snapshot matcher
- Use Mocha root hooks to inject the current test, reset snapshot
  matchers and update snapshot files.
- Add .mocharc.js for packages/cli to register root hooks

Co-Authored-By: Miroslav Bajtoš <mbajtoss@gmail.com>
@bajtos bajtos force-pushed the feat/parallel-snaphots branch from f7d26cf to e16d378 Compare June 26, 2020 06:07
@bajtos bajtos added developer-experience Issues affecting ease of use and overall experience of LB users feature labels Jun 26, 2020
@bajtos bajtos merged commit 862072b into master Jun 26, 2020
@bajtos bajtos deleted the feat/parallel-snaphots branch June 26, 2020 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

developer-experience Issues affecting ease of use and overall experience of LB users feature Internal Tooling Issues related to our tooling and monorepo infrastructore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants