-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add command line option support for Android #597
Add command line option support for Android #597
Conversation
Can someone please restart the Travis CI build? It failed because of some unrelated Maven error. |
@friederbluemle done! |
One thing that bothers me a little with this is that it might be hard to keep in sync with What if you could do this instead:
I think that would simplify the implementation considerably. Also, since release 1.1.5 you can also define cucumber options in the
Wouldn't that just work without your change? |
@friederbluemle I just saw this PR and I am really happy to have this feature soon. fyi: I am currently working on a PR for the android-maven-plugin to also support adding instrumentation args so that one could easily pipe the args all the way through from the pom/cli to the CucumberInstrumentation. |
@SierraGolf / @friederbluemle what do you think about my suggestion to just use Or if that doesn't work (it should) - just use |
I think the environment variable will not work because the environment of the adb call is another than the one in which the actual instrumentation runs. to use just one cucumber.options variable is something I would also prefer, because of the following two reasons:
|
Could you try just to be sure? I would think the environment variables passed to |
@aslakhellesoy Using the As mentioned, passing a parameter that contains spaces requires the rather odd syntax
it would require much more typing with weird quotes:
I am working on another pull request that implements some of the functionality found in Since @SierraGolf / @mfellner / @aslakhellesoy |
Wouldn't that just be? String commandLineArgs = arguments.get("cucumber.options");
if (commandLineArgs != null) {
System.setProperty("cucumber.options", commandLineArgs.replaceAll("'", ""));
} @friederbluemle can you rebase please? also I would like to see some test coverage as soon as we decided which way to go. I know that there are currently no tests for the CucumberInstrumentation class, so let's start now. |
I agree with @friederbluemle that supporting multiple arguments is the correct way to implement it for Android. However @aslakhellesoy's concerns regarding the increased maintenance cost are also sound! I think that in the future there should be a single class containing all possible arguments and their properties (long name, short name, values...) so that RuntimeOptions, RuntimeOptionsFactory and CucumberInstrumentation can just iterate over a list of all arguments and use them as they need to. |
@friederbluemle I'm not following why you need to
Why can't you just
|
Update on the environment variable idea
This has no effect since it is executed in 2 completely different processes on different machines.
This does not work, because it seems to be an illegal format.
Even if you would log into the adb shell first and then run the second command, it would not work because the android runtime is an isolated sandbox. |
@SierraGolf thanks for trying it out. Let's ditch the env var approach then. I still want to explore if we can use Can we try to make |
Why would the
I don' t really get this argument. Why would they need to operate the same way? They do completely different things. The responsibilities of the
Everything else is already provided by the cucumber framework.
I am unsure what you mean by that, can you give an example? Different approach public class PropertyLoadingCucumberInstrumentation extends CucumberInstrumentation {
@Override
public void onCreate(final Bundle bundle) {
loadBundleIntoSystemProperties(bundle);
super.onCreate(bundle);
}
private void loadBundleIntoSystemProperties(final Bundle bundle) {
for (final String key : bundle.keySet()) {
System.setProperty(key, bundle.getString(key));
}
}
} I needed this anyway because I do not have access to the Bundle in my step and hook definitions. Maybe we can add this little wrapper to the module so that users can choose to either take the Btw. here are some example calls of the extended android maven plugin:
|
I think for now we should just implement the simpler solution, which is to pass cucumber.options as a single key-value pair. This requires less code to achieve the goal. Later on the ability to provide individual key-value pairs for every option can still be added additionally. @friederbluemle, can you confirm that using spaces inside the value string requires double quotes? If you can just pass the list of options as a string like, e.g.,
then you can just split the string by spaces and pass it on to RuntimeOptions: String[] args = str.split("\\s+");
RuntimeOptions runtimeOptions = new RuntimeOptions(new Env("cucumber-jvm"), args); @SierraGolf Maven integration looks great. But can we include the functionality of PropertyLoadingCucumberInstrumentation into CucumberInstrumentation? I think the handling of any arguments should be the job of CucumberInstrumentation. I'd rather avoid using a subclass for two reasons:
|
No, don't split by spaces - this would screw up options like this:
See #379 for details. I'll make some changes to String commandLineString = ...;
RuntimeOptions runtimeOptions = new RuntimeOptions(new Env("cucumber-jvm"), commandLineString); |
@aslakhellesoy, you are correct, I missed that. So we can use this instead: Properties properties = new Properties();
properties.setProperty("cucumber.options", arguments.getString("cucumber.options"));
new RuntimeOptions(new Env(properties)); Or a variation thereof with iterating over arguments like in @SierraGolf's example or another variation where System.setProperty is used (in that case Env() or Env(String bundleName) need to be called somewhere I suppose). In any case I would be in favor of first implementing a simple approach of this kind. |
Ok, so with 4ed29b0 in place, this is how you'd do it: new RuntimeOptions(arguments.getString("cucumber.options")); Simple! |
Please keep in mind that the functionality should be as follows:
|
Yeah that is the point, because not everyone might want the bundle piped through to the system properties. I think it is in general a nice feature, but people might be surprised to see that the
I am with you on that, one would need to implement an abstract |
Wow this has turned into a lively discussion.. Thanks everyone for your comments :)
This example of course does not make sense. Sorry about that. You would only need single quotes if a value inside of Cucumber options contains spaces:
Yes that is possible and is implemented in the latest code.
I understand your concern about namespace clashes, but like I said,
Yeah, I get your point about how Cucumber options work in Java. But unfortunately it is not consistent at all with how test runners for Android are expected to work.
I would argue that they do very similar things: They both run test cases. The only difference is that
If that was ever a consideration, I would strongly advise against blindly setting all supplied arguments as system properties. This could have unintended side effects, as other components might use system properties as well. Also, Cucumber options are not the only arguments that can be supplied to this test runner. As I mentioned in an earlier comment, I have reimplemented some of the existing functionality of
Great! One question, can you confirm that these two Cucumber options strings are equivalent:
The reason I'm asking is because only variant (1) works on the command line:
Otherwise we would need to replace single quote with double quote before passing the string. With the latest rebase, both a single Cucumber option strings and separate arguments are supported, with the single string taking precedence over separate arguments. The class annotations have the lowest priority. @aslakhellesoy As you can see in the code, the options are still set as a system property. Could you please advise on how to make use of your new
|
Unix shells will parse both of these in the same way. The shell creates the array I don't know if Windows will parse them the same way. As you have discovered, our own naive shellwords implementation only handles single quotes. I factored it out in 8f01484 and added a couple of ignored failing tests. @friederbluemle do you want to try and make them pass? As you can see from 976111f you can't just replace signle quotes to double quotes. As for the |
I think this issue has already been a little bit overcomplicated. There is now a straightforward solution to fulfill the basic requirement of supporting command line arguments:
new RuntimeOptions(arguments.getString("cucumber.options"));
Then the next step would be to allow a combination of annotation and command line argument. One straightforward way would be to implement a merge interface in RuntimeOptions. Then you could create two RuntimeOptions instances and just Finally, additional support for separate key-value pairs for each cucumber option can be implemented. I think it would be better to declutter this whole issue and work little by little in small incremental changes. |
Sounds good! I'll start to work on that over the next couple of days. |
this.arguments = arguments != null ? arguments : new Bundle(); | ||
} | ||
|
||
private boolean getBooleanArgument(String tag) { |
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 am a little confused now on what the consensus is about multiple/single argument, but wanted to give a hint about this line of code anyway. I think you can inline this method by simply using arguments.getBoolean(tag)
in case the default should be false, or use arguments.getBoolean(tag, true)
in case the default should be true.
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.
Never mind, I just double checked and found out that the Bundle helper methods are not working in this context.
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.
Yeah, unfortunately arguments.getBoolean(tag, true)
cannot be used here, since it casts the value to a Boolean instead of parsing a String.
If a merge interface is implemented as suggested above (options1.merge(options2) etc), can the implementation be such that it supports multiple @CucumberOptions by merging them in a specified order, with later options overriding earlier ones? |
@dimgit could you please create a new issue for |
Done. See #608 |
This adds support to only count the number of scenarios by setting the 'count' argument to true.
This adds support to wait for a debugger to attach at instrumentation start. 'debug' can either be set to true or a number which represents the timeout in milliseconds. Default timeout is 10 seconds.
'debug' support has been added through PR cucumber#613
This adds support to generate EMMA code coverage reports when using CucumberInstrumentation. It is pretty much a 1:1 copy from InstrumentationTestRunner.
This has the same effect as providing the Cucumber option '--dry-run'.
This removes some code duplication in CucumberInstrumentation and InstrumentationArguments.
Added support for legacy
See updated PR description. |
@friederbluemle I know this thread goes on for a while already and that we both want this to get merged because this would unblock us from continuing development etc. but I still think there is a miss-understanding. I think @aslakhellesoy, @mfellner and me want to have a single argument for the cucumber options for now. |
Yes, I think this would be good. Additional features should be added incrementally, we might even create issues for them. |
As mentioned in the comment on Oct 15th, a single argument for the Cucumber options is supported:
Updated the PR description accordingly. But I guess the discussion is more about removing the May I suggest the following at this time:
I am asking this because a significant amount of work has been put into this already, the code is working perfectly fine and is unit tested. |
Hi guys, my team and I are already using a custom build of cucumber-android jar in order to make use of -e tags and -e coverage arguments |
@friederbluemle Sorry, I did not see that both single and multiple arguments are supported. So I am fine. @mfellner / @aslakhellesoy If you guys are fine with it, I will merge this one and throw in some refactorings to add some test coverage. We should also document the arguments in the README and btw. is there any good document which describes what the options do? |
Good idea! This PR is definitely not a place to keep permanent documentation. ;) If someone can point me to a document, I can write something up. |
@SierraGolf (One place where) The options are described is in the Usage.txt, which is displayed on the -h option. @friederbluemle I recon that the Android module README.md is a place to document command line support for Android. |
@brasmusson wow, that is well hidden. I feel that for the cucumber-jvm modules there should be a more prominent place, like on the website, where options and module specific behaviour should be documented. I feel the entry into cucumber world for the JVM languages is quite difficult, especially when you are not coming from the ruby world. |
It's high time we unite our forces to move docs over to the website. It currently lives on https://github.com/cucumber/cucumber.github.com. Feel free to start adding documentation there. |
Add command line option support for Android
@SierraGolf There is a heading for the description of the options on the website (where https://github.com/cucumber/cucumber.github.com is published), but the description is currently only a TODO. I don't even know if I saw it before I pointed to the Usage.txt |
Hi guys, I know this is old, but I am running the command: adb shell am instrument -w -e cucumberOptions "'--tags @settings'" com.rsouza.test/.Instrumentation and it's not generating cucumber reports |
@rafaelaazevedo Have you tried |
@donvigo not, it's returning: I've tried html, pretty, junit, anything just to see the report |
I'm running cucumber tests using calabash: Try "-o" param instead of "--out". |
Please take the discussion up on the mailing list: http://cucumber.io/support This is for bugs and defects only! :) |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This will enable cucumber-android test projects that are started through the activity manager (
adb shell am instrument
) to receive additional Cucumber options from the command line.General
A normal test project would be started like this:
The activity manager (
am
) supports additional extras for theinstrument
command:Multiple extras can be passed like this:
-e key1 value1 -e key2 value2
Supported Cucumber options
-e noDryRun true
-e noMonochrome true
-e noStrict true
Single argument for Cucumber options
A single argument extra for the Cucumber options is also supported:
When using
cucumberOptions
all other Cucumber option extras will be ignored.A few things to note
--strict
would have to be passed like this:-e strict true
-e features [ [FILE|DIR][:LINE[:LINE]*] ]+
-e name "'My Feature'"
-e name Feature1 -e name Feature2
would result in the first name argument being overwritten by the second name argument: The test app would only seeFeature2
.In order to pass options more than once, you would need to separate the values by two dashes
--
, like this:-e name Feature1--Feature2
This pull request also adds support for the following legacy test runner arguments
Note: This was added by PR #613 already.
Asking for comments from the community, especially on the selection of the option value separator. I don't particularly like the two dashes, but it works. Does anyone have a better suggestion?
The requirements are simple: It has to be a character or separator that is
&
and|
will not work),
as in--tags @tag1,@tag2
Thanks