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

fix(ifstep audit trail): fix of ifstep audit trail and context when we use nestedsteps #52

Merged
merged 5 commits into from
Nov 5, 2021

Conversation

endersoncosta
Copy link
Contributor

fix #51

Proposed Changes

  1. If we use nestedsteps without external context, the _stopExecution don't exist, therefore then if we have context, the internal context will be overwrite, else we can use the internal context just to don't break the application.
  2. Today when we execute an IfElse step the audit trail is lost, because we use the return of steps to compose the ifstep audit trail, I changed this to insert the audit trail corretly.

test/ifElse/ifElseStep.js Outdated Show resolved Hide resolved
})
})

describe('using nested steps with context', () => {
Copy link
Member

Choose a reason for hiding this comment

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

how using nested steps with context test is different from using nested steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nestedsteps use the _stopExecution which came from the context, if we overwrite the context with undefined, then the nestedstep will break.

Seeing this, I put an if and made two tests, with context and without context.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but I could not find the difference between these two test cases. It seems they are exercising the same code / scenario. Could you spot to me the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, in the ifElse step, we had step.context = this.context without if.

With this code in this manner, if we performed that code with usecase context, then will work correctly. But if we run the code without usecase context, the context of the step don't has the _stopExecution and will break the application...

I included an If statement to fix this , then this affects the two flows, for this reason I choose run the "same test" to both contexts.

Copy link
Member

Choose a reason for hiding this comment

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

Given there is no difference or new input, the test should be just one. Or force the input to be different.

})
})

describe('using nested steps with context', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but I could not find the difference between these two test cases. It seems they are exercising the same code / scenario. Could you spot to me the difference?

test/ifElse/ifElseStep.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #52 (9bd9488) into master (ff87d0a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   97.45%   97.50%   +0.04%     
==========================================
  Files           6        6              
  Lines         236      240       +4     
==========================================
+ Hits          230      234       +4     
  Misses          6        6              
Impacted Files Coverage Δ
src/ifElse.js 97.29% <100.00%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff87d0a...9bd9488. Read the comment docs.

@jhomarolo
Copy link
Contributor

@endersoncosta is the PR name using the correct semantics? https://github.com/herbsjs/buchu/blob/master/.github/CONTRIBUTING.md

@endersoncosta
Copy link
Contributor Author

@endersoncosta is the PR name using the correct semantics? https://github.com/herbsjs/buchu/blob/master/.github/CONTRIBUTING.md

I used the commitizen... And in the contributing.md I can't understand if has any pattern to this matter.

@dalssoft
Copy link
Member

And in the contributing.md I can't understand if has any pattern to this matter.

Agree

@jhomarolo-vortx jhomarolo-vortx added bug Something isn't working hacktoberfest labels Oct 4, 2021
@endersoncosta
Copy link
Contributor Author

@dalssoft what's left to finish this case?

@dalssoft
Copy link
Member

dalssoft commented Oct 5, 2021

@endersoncosta there is an open issue

Given there is no difference or new input, the test should be just one. Or force the input to be different.

@endersoncosta
Copy link
Contributor Author

Right, the sceneries was different, but I made some complement to add more sceneries with context...

@jhomarolo
Copy link
Contributor

@endersoncosta / @dalssoft Anything pending in this PR?

@dalssoft
Copy link
Member

@endersoncosta need to fix the test code as reviewed before

@endersoncosta
Copy link
Contributor Author

the test has been fixed

})
})

describe('using nested steps with context', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Given there is no difference or new input, the test should be just one. Or force the input to be different.

@dalssoft dalssoft merged commit 8865d26 into herbsjs:master Nov 5, 2021
@jhomarolo
Copy link
Contributor

🎉 This PR is included in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

jhomarolo added a commit to herbsjs/herbs that referenced this pull request Dec 12, 2021
fix buchu bug in audittrail (herbsjs/buchu#52). Add ID field feature in
gotu (herbsjs/gotu#52). Add checker isIterable in suma
(herbsjs/suma#36). Add javascriptIdentifier in suma
(herbsjs/suma#38)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IfStep with nested steps auditTrail bug
4 participants