[TEST] test command line options #18437
Conversation
This commit adds a test for command line options -p, --version and --help closes elastic#16129
brwe
added
>test
| delete node.homeDir | ||
| delete node.cwd | ||
| doLast { | ||
| node.cwd.mkdirs() |
There was a problem hiding this comment.
This seems like a strange thing to do in a clean task.
There was a problem hiding this comment.
I copied that part from here: https://github.com/elastic/elasticsearch/blob/master/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy#L149 I figured the directory has to be created somewhere and did not want to add an additional task for it. But I can totally add that somewhere else. Have a suggestion where a good place would be?
|
I started looking at this this morning before going out for the morning and I'm just getting back to it. I get that testing these in groovy makes sense because we have all the dependencies, but I think this is usually something we want to do in junit tests, right? I mean, should we do it there? Do we just do it in groovy because it is easier there? I'm pretty ok with that but I want to be sure I'm not missing something. |
|
This is a lot of duplication of existing infrastructure just to test output (which should be done in unit tests). In fact, we already have the ability, and do, test these in unit tests. And our integration tests start elasticsearch (and the plugin cli), so I'm not seeing the benefit of this added complexity to the build? |
Right. They use the pidfile pretty extensively irrc. We probably don't need to test that again? Is it that we aren't confident that the unit tests will catch errors with stuff like --version and --help? We have the vagrant tests for lots of that kind of thing but you are right that they don't cover windows at all. |
Can't we have windows vagrant images? |
Maybe? I don't understand the licensing and I'm afraid to ask. Even if we did we'd have to jump through a zillion hoops because bats is, well, bash. |
I think we could restructure the vagrant test infra to allow running something other than bash. Really it is just "ssh to the vagrant image and run something". |
|
Sure! But maybe we're making the perfect the enemy of the good? I mean, if we have time to solve the windows vm problem now then we should do it, but maybe testing these parameters at all in windows is important and maybe something like this is the way to go until we do have the time? Or maybe just running the build with its use of |
|
I barely understand the discussion but will try to answer anyway.
I added this test to check that the startup scripts
We do not seem to use the -p option there. Instead we write that in the config if I understand correctly.
That is what I did on 2.x via hacking the various ant scripts. But I never liked it because then these options are just tested implicitly. I would have preferred a dedicated test on 2.x already but thought at least on master I make it right.
Definitely! I will try and find out where we are with the windows vms. I know there was some discussion about that. Will come back with an update. If that is possible I'd rather have a vagrant test. Again, if I was just missing some smarter unit test option let me know. |
|
We discussed and came to the conclusion it would be better to invest in a proper vagrant test like we do with other systems and then move this test to vagrant tests. It would make more sense because issues on windows pop up every now and then and this test is just one of many we need to implement anyway and we probably don't want to do all with gradle. |
This commit adds a test for command line options -p, --version and --help
closes #16129