Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

BUILTIN_EXPLORER must not require test_package_name #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abuenovanadis
Copy link

Hello this PR is to change this issue:

#47

When you accepted the PR, could I access in bitrise?
Thanks!

Copy link
Contributor

@fadookie fadookie left a comment

Choose a reason for hiding this comment

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

This is on the right track. Did you test this code?

I will test it before merging but please test it on your end first in case there are any bugs. Instructions for testing locally are in the README.

Just from looking at the code I think there may be some stuff you missed. First off test_package_arn also has a validation check which will probably fail in the new code path. Also, it seems best to omit this parameter from the call to the aws cli instead of passing it with an empty value.

@markhu
Copy link

markhu commented Jan 18, 2019

Similar issue with BUILTIN_FUZZ type of test not needing a test_package_name file.
Also, the legend prompt text should be updated to clarify it is the test script, not the binary executable that is being tested.

@abuenovanadis
Copy link
Author

I'm going to try in my local with cli.

@abuenovanadis
Copy link
Author

I setup the same workflow in local, how can i use this new step in dev ?

Thanks!

@fadookie
Copy link
Contributor

@abuenovanadis I am not sure what you mean by "use this step in dev" exactly. If you can run the tests in this repo it should be enough to verify if your changes to the step code actually work.

You can point your bitrise.yml to your fork and/or a specific commit instead of the official step from the steplib. So you may be able to fix this problem for yourself (which is great for testing), but nobody else will benefit unless I merge your changes and cut a new release.

If you could test this and also add BUILTIN_FUZZ to your whitelist that would be great. Thanks.

Copy link
Contributor

@fadookie fadookie left a comment

Choose a reason for hiding this comment

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

needs testing, see comments above

Copy link
Contributor

@elashpear elashpear left a comment

Choose a reason for hiding this comment

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

needs testing, see comments above

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants