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: handle logging step #4751

Closed
wants to merge 4 commits into from
Closed

fix: handle logging step #4751

wants to merge 4 commits into from

Conversation

kobenguyent
Copy link
Collaborator

@kobenguyent kobenguyent commented Jan 17, 2025

Motivation/Description of the PR

const  Step = require('codeceptjs')
const step = new Step('I', `expect "${JSON.stringify(actualValue)}" to not equal "${JSON.stringify(expectedValue)}"`);
output.step(step)
  • handle the case which the step is a string
    [1]  Error (Non-Terminated) | TypeError: step.toCliStyled is not a function | err => { step.status = 'failed' step.endTime = Dat...
    [1] Error | TypeError: step.toCliStyled is not a function undefined...
    [1] <teardown>  Stopping recording promises
    [2]  Starting recording promises

Type of change

  • 🔥 Breaking changes
  • 🚀 New functionality
  • 🐛 Bug fix
  • 🧹 Chore
  • 📋 Documentation changes/updates
  • ♨️ Hot fix
  • 🔨 Markdown files fix - not related to source code
  • 💅 Polish code

Checklist:

  • Tests have been added
  • Documentation has been added (Run npm run docs)
  • Lint checking (Run npm run lint)
  • Local tests are passed (Run npm test)

@kobenguyent kobenguyent requested a review from DavertMik January 17, 2025 16:47
package.json Outdated
@@ -33,7 +33,8 @@
".": "./lib/index.js",
"./els": "./lib/els.js",
"./effects": "./lib/effects.js",
"./steps": "./lib/steps.js"
"./steps": "./lib/steps.js",
"./step": "./lib/step.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

how about adding step export to index.js?

const { step } = require('codeceptjs')

this is how we access all internal API

codeceptjs/steps is userland API which is used inside tests, not custom helpers, or plugins

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds better @DavertMik

@kobenguyent kobenguyent requested a review from DavertMik January 19, 2025 06:37
lib/output.js Outdated
if (step.comment) {
stepLine += colors.grey(step.comment.split('\n').join('\n' + ' '.repeat(4)))
}
try {
Copy link
Contributor

@DavertMik DavertMik Jan 20, 2025

Choose a reason for hiding this comment

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

maybe better would be:

if (step instanceof Step) {
  // ...
} elseif (typeof step == 'string') {
} else {
   // incorrect step
}

try catch is not the best approach here if we know where error can happen and how to avoid it

@kobenguyent kobenguyent requested a review from DavertMik January 20, 2025 04:41
@DavertMik
Copy link
Contributor

Something happened with tests in this 😢

@DavertMik
Copy link
Contributor

DavertMik commented Jan 24, 2025

Hey @kobenguyent , I have a better idea for this case

How about create a Step that can execute arbitrary function

See my implementation in #4781

https://github.com/codeceptjs/CodeceptJS/pull/4781/files#diff-175fae64a79f8903d1998c3068b43193025a23c36cecc50031fc7009e19b0460

here is what you can do:

const  FuncStep = require('codeceptjs/lib/steps/func')
const step = new Step(`expect "${JSON.stringify(actualValue)}" to not equal "${JSON.stringify(expectedValue)}"`);
step.setCallable(...) // actual assertion happens here?
output.step(step)

you can actually add this step to promise chain

const recordStep = require('codeceptjs/lib/steps/record')
const  FuncStep = require('codeceptjs/lib/steps/func')
const step = new Step(`expect "${JSON.stringify(actualValue)}" to not equal "${JSON.stringify(expectedValue)}"`);
step.setCallable(...) // actual assertion happens here?
recordStep(step)

In this case, all steps should are actual Steps
if needed more we can extend from BaseStep to create customized steps

so no need to pass strings

@kobenguyent
Copy link
Collaborator Author

Hey @kobenguyent , I have a better idea for this case

How about create a Step that can execute arbitrary function

See my implementation in #4781

https://github.com/codeceptjs/CodeceptJS/pull/4781/files#diff-175fae64a79f8903d1998c3068b43193025a23c36cecc50031fc7009e19b0460

here is what you can do:

const  FuncStep = require('codeceptjs/lib/steps/func')

const step = new Step(`expect "${JSON.stringify(actualValue)}" to not equal "${JSON.stringify(expectedValue)}"`);

step.setCallable(...) // actual assertion happens here?

output.step(step)

you can actually add this step to promise chain

const recordStep = require('codeceptjs/lib/steps/record')

const  FuncStep = require('codeceptjs/lib/steps/func')

const step = new Step(`expect "${JSON.stringify(actualValue)}" to not equal "${JSON.stringify(expectedValue)}"`);

step.setCallable(...) // actual assertion happens here?

recordStep(step)

In this case, all steps should are actual Steps

if needed more we can extend from BaseStep to create customized steps

so no need to pass strings

Hi @DavertMik thanks for sharing this. So still do we need this PR just to handle the custom helpers for instance that are not yet adapted with new implementation?

@DavertMik
Copy link
Contributor

@kobenguyent if you think it is needed for backwards compatibility with Expect helper I'm ok
but in 4.0 this should be dropped

@kobenguyent
Copy link
Collaborator Author

@kobenguyent if you think it is needed for backwards compatibility with Expect helper I'm ok

but in 4.0 this should be dropped

Oh, if this shall be dropped in 4.0 then I'd say let's exclude this.

@kobenguyent kobenguyent deleted the export-step branch January 26, 2025 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants