Option for enhanced debugging output during testing #561

olofd · 2024-09-18T19:48:11Z

Hello fellow maintainers and old friends (@klippx @tulios), hope all is well!

This PR is a joint effort between me, @J3MPA and @vaibhavar

One thing that we have struggled with a lot with in mappersmith is to know why your installed mock does not match
an outgoing request. This PR aims to change that by giving the developer much more detailed information on why their mock does not match the request. (See attached screenshot).
It should only affect test-code.

We have put the behaviour behind a new flag that you can pass to install({ enhancedDebugging: true })
My thinking is that we might want to dog food it a bit before it would replace the existing behaviour.

Unfortunately the PR includes two dependencies that you might object to, but let's have a discussion on how we can solve that. Should we add them as optionalDependencies and give instructions if enhancedDebugging is enabled?

Let us know what you think.

Olof Dahlbom

changeset-bot · 2024-09-18T19:48:14Z

⚠️ No Changeset found

Latest commit: 6bd36c7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

klippx

Sorry for late review, I am excited to try this out. It is a very cool PR, and I realize that it needs dependencies added for it to work, since mappersmith/test is not a separate package, it's all part of a single published package. So testing suffers from productions requirement of being zero dependency, and adding deps would make production build "look bad" since it is technically not zero dep any more...

But about optionalDependencies, idk how that would work. It would install them anyway unless you did --no-optional so it doesn't help much, even if we update install instructions. But even though, I still do not think this option helps a user to A) having it during development but B) not having it in a production build (as it is today) since they only have one lockfile.

If we really want to keep things zero dependency we have to go for a monorepo and publish a separate package for @mappersmith/test for instance, which is a large effort and beyond the scope here. But in that future state we can have @mappersmith/forge as the production part without dependencies. For now, I think we have to bite the bullet and just allow dependencies.

klippx · 2024-11-05T10:58:01Z

package.json

+  "dependencies": {
+    "diff": "^7.0.0",
+    "tty-table": "^4.2.3"
+  }


How would this work?

https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependenciesmeta

Suggested change

"dependencies": {

"diff": "^7.0.0",

"tty-table": "^4.2.3"

}

"peerDependencies": {

"diff": "^7.0.0",

"tty-table": "^4.2.3"

},

"peerDependenciesMeta": {

"diff": {

"optional": true

},

"tty-table": {

"optional": true

}

}

I guess since this PR aims to be in "experimentation" mode we could consider have it like this for now even though it is a wonkier install experience for the user.

I am not proposing this change, just asking if you have considered it and how you feel about it.

klippx · 2024-11-05T11:08:07Z

src/mocks/mock-request.js

 function MockRequest(id, props) {
  this.id = id

+  this.mockName = props.mockName ? props.mockName : this.id


Maybe interpolate a name from the id, something like:

Suggested change

this.mockName = props.mockName ? props.mockName : this.id

this.mockName = props.mockName ? props.mockName : `Mock#${this.id}`

klippx · 2024-11-05T11:25:11Z

src/test/index.js

+    enhancedDebugging: false,
+  }
+) => {
+  options.enhancedDebugging = enhancedDebugging
  originalGateway = configs.gateway


Can you think if there is a way to "globally" enable enhancedDebugging via configs (GlobalConfigs) object? Would be very nice if a user could do perhaps something like

export const configs: GlobalConfigs = { test: { experimentalEnhancedMockResponseOutputFormat: true } }

klippx · 2024-11-05T14:03:20Z

.gitignore

@@ -11,3 +11,4 @@ tsconfig.tsbuildinfo
 tmp/
 dist/
 .vscode/
+.yarn/cache


Sorry, we are weird, but revert this. And check in the changes of yarn/cache folder :)

Otherwise CI breaks, it wants the cached entries (faster CI).

klippx · 2024-11-05T14:09:47Z

src/test/enhanced-debugging.ts

@@ -0,0 +1,285 @@
+import { sortedUrl, toSortedQueryString } from '../mocks/mock-utils'
+import { Request } from '../request'
+import colors from '@colors/colors'


This dependency is missing, and the test suite fails without it on CI. For you it might work due to a transitive dependency from Karma.

UPDATE:

Actually, this is not working for TSUP which wants to emit ESM output. You can reproduce with yarn tsup.

I think that due to these packages are not ESM, it will be quite hard (dare I say impossible?) to get it to work... 😭

Are there any alternative ESM packages we can use instead?

olofd · 2024-11-05T14:28:53Z

Sorry for late review, I am excited to try this out. It is a very cool PR, and I realize that it needs dependencies added for it to work, since mappersmith/test is not a separate package, it's all part of a single published package. So testing suffers from productions requirement of being zero dependency, and adding deps would make production build "look bad" since it is technically not zero dep any more...

But about optionalDependencies, idk how that would work. It would install them anyway unless you did --no-optional so it doesn't help much, even if we update install instructions. But even though, I still do not think this option helps a user to A) having it during development but B) not having it in a production build (as it is today) since they only have one lockfile.

If we really want to keep things zero dependency we have to go for a monorepo and publish a separate package for @mappersmith/test for instance, which is a large effort and beyond the scope here. But in that future state we can have @mappersmith/forge as the production part without dependencies. For now, I think we have to bite the bullet and just allow dependencies.

No problem with later review. Been equally busy on my end tbh.

It was mostly this dependency discussion I wanted to raise before moving forward with the PR.

Another alternative would ofc. to internalize all the dependent code in a neat place under /test while we are evaluating this?

I could try doing that if you think it’s a good idea.

There is a great cleanliness of having zero deps and I really do not want to ruin that either.

klippx · 2024-11-05T15:21:30Z

As you can see, I tried my best to get this branch working in #563

But even if the mappersmith PR looks green, the smoke test fails when trying to use mappersmith/test from an ESM project. I have tested this in our ESM monorepo and it is exactly the same error.

This means we have to solve this problem before merging. I can try to get it working in it's current state, but it might be best if you find ESM compatible options and relieve me from the stress and pain of trying to cover up of the anti pattern of using non-ESM modules in a library whose goal it is to emit ESM.

klippx · 2024-11-05T15:25:25Z

@olofd and Jens and Vaibav, Feel free to join mappersmith discord server to get quicker discussion and a way to ping each other https://discord.gg/TfAk625Z

olofd changed the title ~~[WIP] Test improvements~~ [WIP] Option for enhanced debugging output durign testing Sep 19, 2024

olofd changed the title ~~[WIP] Option for enhanced debugging output durign testing~~ [WIP] Option for enhanced debugging output during testing Sep 19, 2024

olofd force-pushed the test-improvements branch from ef79275 to b64f4bb Compare September 19, 2024 21:44

Initial commit

ede2052

olofd force-pushed the test-improvements branch from ab7434a to ede2052 Compare September 19, 2024 21:53

Olof Dahlbom added 2 commits September 20, 2024 00:08

Change name of function

cab408f

Fix incorrect pointer

6bd36c7

klippx self-requested a review November 5, 2024 10:47

klippx reviewed Nov 5, 2024

View reviewed changes

klippx changed the title ~~[WIP] Option for enhanced debugging output during testing~~ Option for enhanced debugging output during testing Nov 5, 2024

klippx reviewed Nov 5, 2024

View reviewed changes

klippx mentioned this pull request Nov 5, 2024

[Shadow PR] Option for enhanced debugging output during testing #563

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option for enhanced debugging output during testing #561

Option for enhanced debugging output during testing #561

olofd commented Sep 18, 2024 •

edited

Loading

changeset-bot bot commented Sep 18, 2024 •

edited

Loading

klippx left a comment

klippx Nov 5, 2024 •

edited

Loading

klippx Nov 5, 2024

klippx Nov 5, 2024 •

edited

Loading

klippx Nov 5, 2024

klippx Nov 5, 2024 •

edited

Loading

olofd commented Nov 5, 2024

klippx commented Nov 5, 2024

klippx commented Nov 5, 2024

-  "dependencies": {
-    "diff": "^7.0.0",
-    "tty-table": "^4.2.3"
-  }
+  "peerDependencies": {
+    "diff": "^7.0.0",
+    "tty-table": "^4.2.3"
+  },
+  "peerDependenciesMeta": {
+    "diff": {
+      "optional": true
+    },
+    "tty-table": {
+      "optional": true
+    }
+  }

	this.mockName = props.mockName ? props.mockName : this.id
	this.mockName = props.mockName ? props.mockName : `Mock#${this.id}`

Option for enhanced debugging output during testing #561

Are you sure you want to change the base?

Option for enhanced debugging output during testing #561

Conversation

olofd commented Sep 18, 2024 • edited Loading

changeset-bot bot commented Sep 18, 2024 • edited Loading

⚠️ No Changeset found

klippx left a comment

Choose a reason for hiding this comment

klippx Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

klippx Nov 5, 2024

Choose a reason for hiding this comment

klippx Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

klippx Nov 5, 2024

Choose a reason for hiding this comment

klippx Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

olofd commented Nov 5, 2024

klippx commented Nov 5, 2024

klippx commented Nov 5, 2024

olofd commented Sep 18, 2024 •

edited

Loading

changeset-bot bot commented Sep 18, 2024 •

edited

Loading

klippx Nov 5, 2024 •

edited

Loading

klippx Nov 5, 2024 •

edited

Loading

klippx Nov 5, 2024 •

edited

Loading