feat(serialize): remove surrial #2877
Conversation
|
I did forget to push my changes to github 😅. This actually looks almost the same as what I have done, so I believe it is good. Since 2 people independently have done the same work |
|
Ah sorry, I wanted to move forward with this as it is one of the last things that needs to be done before we can release v5. 🤷♂️ I'm busy with a small piece of code to verify that your stryker config is fully serializable. If not, I want to give a warning. Something like:
This is what I currently have: |
|
working on printing messages |
|
@nicojs good? |
|
Ow sorry, I was thinking just use |
|
But that works too :D
…On Thu, 6 May 2021, 16:34 Nico Jansen, ***@***.***> wrote:
Ow sorry, I was thinking just use paths.join('.') (basic json path). We
shouldn't spent too much time on it 🤷♂️
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2877 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGZLQLX7FIVBNZNANXJNSBDTMKLGZANCNFSM44F2HUAQ>
.
|
|
I think this is ready for review. @kmdrGroch would you mind taking a look? 👀 |
| @@ -7,7 +7,7 @@ | |||
| "checkCoverage": true, | |||
| "reporter": "html", | |||
| "reportDir": "reports/coverage", | |||
| "exclude": [ | |||
| "testResources/**/*.js" | |||
| "include": [ | |||
There was a problem hiding this comment.
what happened there? Exclude no longer needed?
There was a problem hiding this comment.
No, we should have used an include all along. I noticed it because of this failing pipeline. Coverage was calculated on the unit test files themselves, instead of the source files 😅
We're also using an include in all of our .nycrc.json files, don't know why not here.
| @@ -221,6 +221,11 @@ | |||
| "description": "decide whether or not to log warnings when a preprocessor error occurs. For example, when the disabling of type errors fails.", | |||
| "type": "boolean", | |||
| "default": true | |||
| }, | |||
| "unserializableOptions": { | |||
| "description": "decide whether or not to log warnings when a configuration options is unserializable. For example, using a /regex/ in your config options.", | |||
There was a problem hiding this comment.
i believe "configuration options are unserializable"
| @@ -127,7 +127,7 @@ export class ChildProcessProxy<T> implements Disposable { | |||
|
|
|||
| private listenForMessages() { | |||
| this.worker.on('message', (serializedMessage: string) => { | |||
| const message: ParentMessage = deserialize(serializedMessage, [File]); | |||
| const message: ParentMessage = deserialize(serializedMessage); | |||
There was a problem hiding this comment.
Potentially could use
const message = deserialize<ParentMessage>(serializedMessage);to keep it consistent as it is in other files
plus that's why we have
function deserialize<T>(s: string): T| return options; | ||
| } | ||
|
|
||
| function markUnknownOptions(options: StrykerOptions, schema: JSONSchema7, log: Logger): void { |
There was a problem hiding this comment.
is explicit return type needed here? We are not exporting that function
There was a problem hiding this comment.
Nope, it isn't. But it is allowed, isn't it? I'll remove these explicit return types.
There was a problem hiding this comment.
Explicit return types are bugprone. So if we don't have to, better let's not do it :D
| } | ||
| } | ||
|
|
||
| function markUnserializableOptions(options: StrykerOptions, log: Logger): void { |
There was a problem hiding this comment.
same here. Explicit return type is no needed IMO
packages/util/package.json
Outdated
| "@types/lodash.flatmap": "~4.5.6", | ||
| "@types/node": "^14.0.1" | ||
| "@types/node": "^14.0.1" |
There was a problem hiding this comment.
all around me are familiar faces, trailing spaces, trailing spaaceesss
| export function findUnserializables(thing: unknown): UnserializableDescription[] | undefined { | ||
| switch (typeof thing) { | ||
| case 'number': | ||
| return isNaN(thing) || thing === Infinity || thing === -Infinity |
There was a problem hiding this comment.
nice catch! Instead of checking +- Infinity you can do !isFinite(thing)
There was a problem hiding this comment.
Wow, great catch. Didn't even know about isFinite. It also catches NaN apparently, so replaced all 3 checks with 1 isFinite check 👍 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isFinite#return_value
There was a problem hiding this comment.
Wow... i knew about isFinite but didn't know it catches NaN as well :D
we all learn hahaha
| @@ -91,13 +91,14 @@ | |||
| "source.fixAll.eslint": true | |||
| }, | |||
| "cSpell.words": [ | |||
There was a problem hiding this comment.
are you even using cSpell.words? 🤔
There was a problem hiding this comment.
I'm using it. But indeed, we should add it to our recommended plugins. It is awesome! Been adding words in our config (using ctrl+. and choosing "add this word to workspace").
|
In total looks good. Only small details IMO :) |
Replace surrial with
JSON.stringifyandJSON.parseto serialize and deserialize the messages betweenCheckerandTestRunnerworker processes.This has a couple of advantages:
✅ Improves performance (slightly).
✅ Removes a (small) security risk where an
eval~ish way was used to deserialize.✅ Reduces weird behavior in edge cases where you provide functions in Stryker options.
✅ Reduces maintenance load by removing a fringe dependency (that I maintain, but still 😅).
However, this does mean that an undocumented feature disappears (a small, but breaking change).
This is why Stryker will provide an optional warning:
BREAKING CHANGE: Having a non-JSON-serializable value in your configuration won't be sent to the child process anymore. If you really need them in your test runner configuration, you should isolate those values and put them in test runner-specific config files, loaded by the test runner plugin itself, for example, jest.config.js, karma.conf.js, webpack.config.js.
Closes #2814