-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: added journey and step annotations #880
Conversation
@shahzad31 what's the use case for skipping a step like this (as opposed to deleting it, commenting it out)? Is it to keep the structure as it was before a change (so step n is always step n, even if step n-1 is removed)? Do you envisage this being a temporary thing a user would do, or permanent? |
@paulb-elastic it's the most familiar way to in testing frameworks to provide an option like this instead of commenting out , it's especially useful when testing journeys locally. It's a very familiar syntax for especially jest developers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments.
- What do you think about adding support for journey.skip which is being asked here - Enable journey.skip to skip tests #404. We need to handle the skipped journeys from being pushed in project based monitor workflow
- Need tests for the skipped steps
src/cli.ts
Outdated
@@ -185,6 +185,7 @@ program | |||
'the target Kibana spaces for the pushed monitors — spaces help you organise pushed monitors.' | |||
) | |||
.option('-y, --yes', 'skip all questions and run non-interactively') | |||
.option('-f, --force', 'Ignore warnings and force push') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would reuse the -y
, we already use this for ignoring some of the users changes on local and forcefully pushing the project monitors. Lets not introduce two things to do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking through this again. This would end up costing user and doing harm than good. Lets just error in case of skipped journey and figure out a solution if there is a need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have remove the -f
src/core/index.ts
Outdated
@@ -57,13 +63,80 @@ export const journey = wrapFnWithLocation( | |||
} | |||
); | |||
|
|||
export const step = wrapFnWithLocation( | |||
journey.skip = wrapFnWithLocation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much duplicated. Both of the methods .skip and .only can be invoked by having a common function that does the same thing.
func addAnnotation(name: 'only' | 'skip' , args) {
const j = new Journey(...args);
j[name]= true;
runner.addJourney(j);
}
journey.only = addAnnotation('only')
journey.skip = addAnnotation('skip')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/core/index.ts
Outdated
/** | ||
* Failure of soft step will not skip rest of the steps | ||
*/ | ||
step.soft = wrapFnWithLocation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Been thinking about this today, I am not convinced that this makes sense. Feels like we are trying to work around the expect.soft
problem. Lets re-explore this in a separate PR. Mainly suggesting because once we have the expect.soft support this becomes useless.
If the devs want to complete a step without failing, It would be more ideal to either comment out then failing method expect.blah
or change that call to a soft assertion. I dont think anyone would have a step just for a assertion and would call .soft
on that whole step instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think both are separate use case. This just means user doesn't want a step to skip the rest of journey.
step.soft
and expect.soft
can co-exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Shahzad, I am really not convinced on the soft usecase for step. Would you suggest some usecase?
A step can error only if there are any expect statements, otherwise it cant unless its a JS bug which users can figure out if they run locally
if users want to soft assert, they can do expect.soft or so if we cannot add support via the PW expect. Lets do it as followup call, unless there is a need for the step.soft. We would be in a better position to add it later than to deprecate it based on what I said above.
Co-authored-by: Vignesh Shanmugam <vignesh.shanmugam22@gmail.com>
…into add-skip-step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a look today, There are some typings issue and reporter issues. Will push a fix later today.
@shahzad31 I pushed a fix that deals with few things
|
a767eb2
to
685559e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Support syntax like
Following annotation have been added
journey.only
will only run this journeyjourney.skip
will skip this journeystep.skip
step.soft
failure of this step will not abort rest of the journeystep.only
-f --force
if journey is skipped , then while pushing it will error out, but can be ignore with this flagexample result