Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests(smoke): deep clone expectations. do not use p.stdout #10361

Merged
merged 3 commits into from
Feb 25, 2020

Conversation

connorjclark
Copy link
Collaborator

Found two issues when trying to consume this internally.

  1. we clone expectations out of an abundance of caution, but we lose regexps currently. need a regexp replacer/reviver
  2. process.stdout doesn't play nice with how I need to use it. just replaced it with a simple console.log.

@connorjclark connorjclark requested a review from a team as a code owner February 21, 2020 01:59
@connorjclark connorjclark requested review from exterkamp and removed request for a team February 21, 2020 01:59
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems harmless enough, but I want someone with more JS experience than me to say this pattern is makes sense.

@exterkamp
Copy link
Member

@patrickhulce I choose you!

/**
* @param {Smokehouse.SmokehouseLibOptions} options
*/
async function smokehouse(options) {
const {urlFilterRegex, skip, modify, ...smokehouseOptions} = options;

/** @type {Smokehouse.TestDfn[]} */
const clonedTests = JSON.parse(JSON.stringify(smokeTests));
const clonedTests = deepCopy(smokeTests);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just dev dependency why not use just use _.cloneDeep? :) https://lodash.com/docs/4.17.15#cloneDeep

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we decide to keep and maintain this ourselves instead I expect rigorous tests and edge cases ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

@connorjclark connorjclark changed the title smokehouse(lib): deep clone expectations. do not use process.stdout tests(smoke): deep clone expectations. do not use process.stdout Feb 22, 2020
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@connorjclark
Copy link
Collaborator Author

@paulirish what up with pr lint

@connorjclark connorjclark reopened this Feb 24, 2020
@connorjclark connorjclark changed the title tests(smoke): deep clone expectations. do not use process.stdout tests(smoke): deep clone expectations. do not use p.stdout Feb 25, 2020
@connorjclark connorjclark merged commit ff34be1 into master Feb 25, 2020
@connorjclark connorjclark deleted the smokehouse-lib-fix branch February 25, 2020 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants