Skip to content
This repository has been archived by the owner on Jun 25, 2023. It is now read-only.

Acceptance tests: cucumber #182

Closed
wants to merge 1 commit into from
Closed

Conversation

njam
Copy link
Contributor

@njam njam commented Apr 24, 2016

Continuation of #144, part of #136

Run with:

bundle exec rake features

@njam njam mentioned this pull request Apr 24, 2016
@njam njam self-assigned this Apr 24, 2016
@njam njam mentioned this pull request Apr 24, 2016
@njam
Copy link
Contributor Author

njam commented Apr 24, 2016

I've rewritten the cucumber step definitions to check DNS lookups to be based on Ruby libraries. The check to query against Landrush uses Resolv::DNS and the check to query on the operating system level uses Addrinfo.

I think this way it should be cross-platform compatible. On OSX the tests run fine, could you test it on Windows? Note that the example with ubuntu/wily64 fails because of a bug in Landrush.

@hferentschik @fh wdyt?

@hferentschik
Copy link
Contributor

hferentschik commented Apr 26, 2016

I've rewritten the cucumber step definitions to check DNS lookups to be based on Ruby libraries. The check to query against Landrush uses Resolv::DNS and the check to query on the operating system level uses Addrinfo.

Nice. I'll have a look tomorrow.

could you test it on Windows

Will do

@hferentschik
Copy link
Contributor

@njam, I tried these changes locally and it might be that the Cucumber approach is maybe even better than vagrant-spec. A couple of things though.

When trying to run the tests for the first time on my OS X machine, I got:

 ubuntu/wily64   | virtualbox |
      expected "bundle exec vagrant up --provider virtualbox" to be successfully     executed (RSpec::Expectations::ExpectationNotMetError)
      features/dns_resolution.feature:24:in `When I successfully run `bundle exec     vagrant up --provider virtualbox`'
      features/dns_resolution.feature:16:in `When I successfully run `bundle exec     vagrant up --provider <provider>`'

Failing Scenarios:
cucumber features/dns_resolution.feature:23 # Scenario Outline: booting a box,     Examples (#1)
cucumber features/dns_resolution.feature:24 # Scenario Outline: booting a box,     Examples (#2)

2 scenarios (2 failed)
10 steps (2 failed, 6 skipped, 2 passed)
0m44.665s

The VM would not even comes up. The reason is that ResolverConfig tries to add an entry to /etc/resolver/ and needs a password for executing in sudo mode. After the first time it works. I think there are a couple of things at play here. First the sudo password is cached, but also /etc/resolver/my-tld never gets cleaned up. Would there be a way handle the sudo part somehow via the test? Or maybe it would be better to change the code in a way that it does not even create anything in /etc/resolver under test. WDYT?

Once the VM actually starts, I am seeing:

    Examples: 
      | box             | provider   |
      | debian/jessie64 | virtualbox |
      | ubuntu/wily64   | virtualbox |

      expected: "10.10.10.123"
           got: "10.0.3.1"

      (compared using ==)
       (RSpec::Expectations::ExpectationNotMetError)
      ./features/step_definitions/dns.rb:7:in `/^the hostname "([^"]+)" should     resolve to "([^"]+)" on the internal DNS server$/'
      features/dns_resolution.feature:24:in `Then the hostname "my-host.my-tld"     should resolve to "10.10.10.123" on the internal DNS server'
      features/dns_resolution.feature:17:in `Then the hostname "my-host.my-tld"     should resolve to "10.10.10.123" on the internal DNS server'

Failing Scenarios:
cucumber features/dns_resolution.feature:24 # Scenario Outline: booting a box,     Examples (#2)

2 scenarios (1 failed, 1 passed)
10 steps (1 failed, 2 skipped, 7 passed)
1m59.587s

Is this the bug you are referring to? Any idea why 10.0.3.1 is returned? Is there an issue for that?

end

After do |_scenario|
Komenda.run('bundle exec vagrant landrush stop', fail_on_fail: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is Komenda about in this case? What does it add to the mix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a simple way to run a shell command. I couldn't easily find something exposed by Aruba (which might be nicer?).

@hferentschik
Copy link
Contributor

@njam are you running Linux or OS X?

@njam
Copy link
Contributor Author

njam commented May 1, 2016

are you running Linux or OS X?

OSX

Or maybe it would be better to change the code in a way that it does not even create anything in /etc/resolver under test. WDYT?

I was thinking about this as well. It feels a bit strange that we're relying on global state that is persisted on the operating system for these tests. But then on the other hand these are acceptance tests, and their purpose is to verify that Landrush works correctly, in combination with the operating system. I think it's the only way to let Landrush do what it usually does if we want to determine if that works or not.

needs a password for executing in sudo mode.

Hmmm I see. Seems like the way aruba executes the process as a "leader" which means it doesn't have a tty?
I think it would be best if we would ask for sudo, because we don't know what happens within Landrush for the current platform (linux, osx, windows). Should we look into ways to make Aruba commands have a TTY?

Is this the bug you are referring to? Any idea why 10.0.3.1 is returned? Is there an issue for that?

Yes, IP detection on the ubuntu machine doesn't work correctly. It picks up the "wrong" interface's address. There's no specific ticket about this configuration. But I guess we're picking the wrong interface in a lot of configurations. I was referring to #122 as a potential solution.

@hferentschik
Copy link
Contributor

I was thinking about this as well. It feels a bit strange that we're relying on global state that is persisted on the operating system for these tests. But then on the other hand these are acceptance tests, and their purpose is to verify that Landrush works correctly,

Sure, its a tricky one. But if we really also modify the state of the host, we also need to make sure that the changes are reset after the test. BTW, if the aim is to test the host setup as well, then the test (at least for the OS X case) is not quite right. The DNS resolution is against the Landrush DSN server port (10053), not the default DNS port. On OS X one would need to check against the default DNS port to verify whether the DNS configuration change via /etc/resolver worked.

For Linux and Windows the host configuration is not yet automated, so there one needs to verify against port 10053. I am working on some automation for the host configuration on Windows, but that is still a bit off.

Personally, I think it might be easier for now to offer a way to skip host configuration updates (add something like config.landrush.skip.host.dns.config) and test "just" that the Landrush DNS server works.

This removes the need to proper test cleanup (which will differ by OS) and it also works around the sudo permission problem.

We can then evolve the tests over time.

Yes, IP detection on the ubuntu machine doesn't work correctly. It picks up the "wrong" interface's address.

Ok. I see.

BTW, I will be traveling next week, so I won't be so responsive, but it would be great if we could get something worked out for the week after next, so that we can move on with cutting a release. WDYT?

@njam
Copy link
Contributor Author

njam commented May 2, 2016

if the aim is to test the host setup as well, then the test (at least for the OS X case) is not quite right.

I think it is correct actually, as it is testing both cases:

  • the hostname "([^"]+)" should resolve to "([^"]+)" on the internal DNS server: Checks that Landrush's DNS server resolves to the desired address
  • the hostname "([^"]+)" should resolve to "([^"]+)" on the host: Checks that Ruby's standard way of resolving an address has the same result. On OSX this will be using mDNS and /etc/resolver.

config.landrush.skip.host.dns.config sounds like an idea. I'm just a bit concerned that we will be skipping a lot of important functionality that would be great to cover with an acceptance test.

I would rather suggest to keep the check about the resolution on the default resolver in the test. These tests are not running in CI, but are rather meant for us developers, to have a tool to verify that everything works. So one could argue that if somebody is running those tests on Windows, they should also set up DNS resolution. And if they don't they will understand why the tests fail and they can ignore that if they want to.

wdyt?

@hferentschik
Copy link
Contributor

I think it is correct actually, as it is testing both cases:

You are right. Sorry. So the problem is then actually the other way around. On Linux and Windows these tests will fail, because there the host visibility is not setup automatically.

I would rather suggest to keep the check about the resolution on the default resolver in the test. These tests are not running in CI, but are rather meant for us developers, to have a tool to verify that everything works. So one could argue that if somebody is running those tests on Windows, they should also set up DNS resolution. And if they don't they will understand why the tests fail and they can ignore that if they want to.

I see your point. As you say, for now these tests will only be for development and it is fair to assume that the developer has to set up host visibility manually for the OSes where it is not supported out of the box.

That we cannot run these tests on CI is a shame, but that would require that we find a CI provider which either offers bare metal CI servers or setups where one can run virtualization within virtualization. Travis-CI at least does not allow to run VirtualBox on top of their CI environment.

@njam, do you think we can at least address the sudo issue? Also, should we really add 'ubuntu/wily64' as an example even though we know it fails? I think we should also expand the README in regards to who should use the cucumber tests and how to use them (aka the need to configure DNS on the host for Linux and Windows). WDYT?

@hferentschik
Copy link
Contributor

I went ahead and merged the pull request with some minor modifications. It works on OS X (with the potential sudo issue) and Linux. On the latter one needs to manually do the dnsmasq configuration. On Windows it won't work at all atm :-( due to a bug in Aruba - cucumber/aruba#387

vagrant-spec actually would work on Windows (apart of the fact that one would need to also adjust all the Unix type commands in the vagrant-spec based pull request), however, I think the syntax and setup is better with Cucumber and Aruba. Also the Aruba community seems to be active and interested in resolving the problem. So hopefully the test will work in the near future.

@njam, thanks for the pull request.

@njam
Copy link
Contributor Author

njam commented May 12, 2016

👍 Thanks!

Travis-CI at least does not allow to run VirtualBox on top of their CI environment.

Agree, I don't know of any free service that would allow for this. We would probably need to spend some money and set up a Jenkins which does that for us.

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.

2 participants