Skip to content

Conversation

@napoles-mk
Copy link
Contributor

@napoles-mk napoles-mk commented Apr 5, 2022

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 60

Questions:
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

@napoles-mk
Copy link
Contributor Author

@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.
Both functionalities are left, reading values from json file and using parameter on mkcli command for user convenience.

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.

@napoles-mk
Copy link
Contributor Author

@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.

Copy link

@jesus-serranop jesus-serranop left a 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.

Comment on lines +1 to +2
{
"high": 90,

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.

Copy link
Contributor Author

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.

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.

3 participants