-
Notifications
You must be signed in to change notification settings - Fork 0
Apply success criteria to return exit code #25
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
base: master
Are you sure you want to change the base?
Conversation
|
@renan-ua, @jesus-serranop I have applied changes to read also from a json file as discussed. Please take a look. I also updated description on this PR. Tested this on circleci: https://app.circleci.com/pipelines/github/muuklabs/ci_integration/150/workflows/f6711569-2519-4295-9025-95e244200769 This branch is based on master, once you give the go ahead, I think we will need a new branch based on enabled-video branch also. |
|
@renan-ua, @jesus-serranop , @nilson-lagos any comments on the review? @jesus-serranop do you think we could have Monica to try this code? In case they have a testing environment. Notice this is not the branch with video enabled. |
jesus-serranop
left a comment
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.
Just a single comment.
| { | ||
| "high": 90, |
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 it will be good to add a comment here to indicate what those values mean.
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 @jesus-serranop. This values are only demonstrative to be changed per customer needs. Also it would be a little problematic to have comments on the json itself, they will be taken as data.
This PR is not intended to merge at all, but created for getting a review process.
Summary of the problem/fix:
Customer wants to run mkcli on circleci integration, they run the test suite by hashtag and for some tests they do not need the whole suite to be passed but instead, getting a subset of test passed is enough for them to classify the suite execution as success.
This change on the executor is to allow customer to specify a success criteria to define percentage rate of passed tests needed to return a 0 code from executor (success code).
This percentage rate is defaulted to 80 but can be changed with a new argument parameter for mkcli:
Added this sentence on help:
-criteria CRITERIA (Optional). Minimal rate of passed tests to return success code. Value from 0 to 100. Default 80
This is an example using the new parameter:
python3 mkcli.py -p hashtag -t uno -criteria 60Questions:
Does the PR have any dependency with BE or FE?
no
In case of a dependency, could you describe what would happen if not delivered together? (eg. FE will crash, code will not compile, etc)
No dependency
Did you add a new lib package (package.json)? If so, did you update the open source spreadsheet?
No.
Did you link the defect to this PR (Github only)
NA, Issue is: https://github.com/muuklabs/prototype/issues/1427
If you are adding a new FE view or any significant UI change, have you shared the new UI to the team already?
no
Did you include or update the unit tests?
No.
Did you execute the npm tests ?
NA
Did you test it in both environments (dev/prod)?
NA
if you update the routes... Did you update the sample.txt ?
No