test: Add check-subscriptions to test the Cockpit UI by mvollmer · Pull Request #2091 · candlepin/subscription-manager

mvollmer · 2019-05-24T12:56:29Z

This also brings the necessary infrastructure.

mvollmer · 2019-05-24T12:59:23Z

The new test fails with

error: error during autoattach 
log: Error parsing D-Bus error message:  Unexpected token S in JSON at position 0
log: Returning original error message:  Server error attempting a POST to /subscription/consumers/5b470261-6212-4561-b549-3228e6a58005/entitlements returned status 404

I think the URL should be /candlepin/consumers/... since the test runs against a custom Candlepin instance that is configured like that. I check this a bit more...

candlepin-bot · 2019-05-24T13:00:05Z

Can one of the admins verify this patch?

mvollmer · 2019-05-24T13:26:43Z

The code attempts to save the right config, it seems, but it doesn't seem to have any effect.

> debug: saving: server.hostname {"t":"s","v":"10.111.113.5"}
> debug: saving: server.port {"t":"s","v":"8443"}
> debug: saving: server.prefix {"t":"s","v":"/candlepin"}
> debug: auto-attaching
> debug: 
> error: error during autoattach 
> log: Error parsing D-Bus error message:  Unexpected token S in JSON at position 0
> log: Returning original error message:  Server error attempting a POST to /subscription/consumers/dc63bffb-c9cc-4cc9-b914-7beb2e2839ca/entitlements returned status 404

The daemon says:

INFO [rhsm.connection:924] Connection built: host=10.111.113.5 port=8443 handler=/subscription auth=identity_cert ca_dir=/etc/rhsm/ca/ insecure=True
May 24 09:15:30 localhost.localdomain rhsm-service[1804]:  INFO [rhsm.connection:638] Response: status=404, request="POST /subscription/consumers/dc63bffb-c9cc-4cc9-b914-7beb2e2839ca/entitlements"

So the daemon does indeed use the wrong value for "handler".

The config file itself does not look as expected:

[root@localhost ~]# cat /etc/rhsm/rhsm.conf 
[server]
prefix = /candlepin[root@localhost ~]#

Only the handler/prefix made it through and there is no newline at the end.

Doing a manual D-Bus call at this point results in the same error message as above:

# gdbus call -y -d com.redhat.RHSM1 -o /com/redhat/RHSM1/Attach -m com.redhat.RHSM1.Attach.AutoAttach '' '{}' ''
Error: GDBus.Error:org.freedesktop.DBus.Python.dbus.exceptions.DBusException: Server error attempting a POST to /subscription/consumers/dc63bffb-c9cc-4cc9-b914-7beb2e2839ca/entitlements returned status 404

Restarting the daemon changes that:

# systemctl restart rhsm
# gdbus call -y -d com.redhat.RHSM1 -o /com/redhat/RHSM1/Attach -m com.redhat.RHSM1.Attach.AutoAttach '' '{}' ''
Error: GDBus.Error:org.freedesktop.DBus.Python.dbus.exceptions.DBusException: [Errno -2] Name or service not known

The daemon says:

INFO [rhsm.connection:924] Connection built: host=subscription.rhsm.redhat.com port=443 handler=/candlepin auth=identity_cert ca_dir=/etc/rhsm/ca/ insecure=False
ERROR [rhsmlib.dbus.objects.attach:63] [Errno -2] Name or service not known

So it has picked up the new prefix/handler but reverted back to the default host, as could be expected from rhsm.conf.

Thus, I don't think this is a bug with the code in cockpit/ or with test/check-subscriptions but rather with the Config.Set D-Bus API.

mvollmer · 2019-05-24T13:30:20Z

The config file itself does not look as expected:

I should point out that all the previous content of the config file is gone, too. That is also not expected. :-)

jirihnidek · 2019-05-27T12:18:04Z

I think the URL should be /candlepin/consumers/... since the test runs against a custom Candlepin instance that is configured like that. I check this a bit more...

Yes, that's true. When candlepin is deployed without modificated configuration files, then the URL begins with /candlepin and not /subscription

jirihnidek · 2019-05-27T12:35:34Z

@mvollmer I'm almost sure that our QE team have similar tests.

mvollmer · 2019-05-27T13:56:56Z

The problem was that subscription-client.js was setting the config values and calling AutoAttach all at the same time. One could argue that the daemon needs to be able to deal with that and serialize the execution of the method calls as needed, but we can also change the client to make them sequentially. Also, a restart of rhsm seems to be necessary.

I am making progress with adapting the test from Cockpit, and my plan right now is to continue until I can get it green.

At that point, you could merge this and we add this repo to the Cockpit CI config and the test would run automatically for every PR. Wdyt?

mvollmer · 2019-05-27T13:57:50Z

I'm almost sure that our QE team have similar tests.

Did it also find the issue with using a custom url?

mvollmer · 2019-05-28T12:44:09Z

Okay, the tests are green now when I run them locally. I have included the necessary fixes here.

The "Don't concurrently set config and attach" fix is argueably a workaround for a bug in rhsm. It should just cope with overlapping Config.Set calls. I think I'll open an issue about that.

mvollmer · 2019-05-29T13:18:03Z

Here is a demo PR of how this would look in action: mvollmer#1

The PR deliberately breaks one of the tests, so the failure is expected.

mvollmer · 2019-06-05T08:03:12Z

I have changed check-subscriptions around a bit. The old sequence of steps didn't make a lot of sense to me, tbh...

jstavel · 2019-06-07T11:35:26Z

@mvollmer I'm almost sure that our QE team have similar tests.

Yes we have similar tests. On the other hand I suppose it should be easy to reuse tests written this way in our testware.

jirihnidek · 2019-06-10T12:28:01Z

@mvollmer Thanks for your work on this PR. Should we do PR review now or do you still want to work on this PR?

mvollmer · 2019-06-10T12:35:55Z

Should we do PR review now

Please start the review. Thanks!

jirihnidek · 2019-06-10T12:39:26Z

@mvollmer ok :-)

jirihnidek

Hi, thanks for your PR.

I'm little bit confused from changes in Javascript code. Does it include bug fix (it seem so)? You mentioned some technical issues during testing this PR. If changes in this PR contains bug fix, then please open another PR containing only bug fix and please fill Bugzilla bug report (commit message should begin with BZ bug number).

I wasn't able to test it much, because I don't have Fedora (Python 3) on my PC and creating of VM requires Python 3. I will try to test it later.

jirihnidek · 2019-06-11T07:32:31Z

test/run

+bots:
+	git fetch --depth=1 https://github.com/cockpit-project/cockpit.git
+	git checkout --force FETCH_HEAD -- bots/
+	git reset bots


It requires Python 3 and Python 2 is not the option. It should be noted somewhere in the README.md

We also have Vagrant environment. Could it be possible to use existing Vagrant machine or could it be possible to create new testing machine using Vagrant?

We also have Vagrant environment. Could it be possible to use existing Vagrant machine or could it be possible to create new testing machine using Vagrant?

I'll check that out. The immediate goal here is to integrate the subscription-manager UI tests into the Cockpit CI machinery, because that would be relatively easy. (One might say that it's done already. :-)

Does your Jenkins CI machinery use these Vagrant images? If your are more comfortable with running Cockpit UI tests there, we can certainly work on that as well.

The Cockpit CI integration here has the downside that it only tests the new Cockpit code; it doesn't build and install a new rhsmd, for example. I think that's a significant drawback, and if going with Jenkins would fix that, that would be a strong argument.

I'll check that out. The immediate goal here is to integrate the subscription-manager UI tests into the Cockpit CI machinery, because that would be relatively easy. (One might say that it's done already. :-)

I can see the reason, why you decided to use something else than Vagrant.

Does your Jenkins CI machinery use these Vagrant images? If your are more comfortable with running Cockpit UI tests there, we can certainly work on that as well.

The definitions of Jenkins jobs are here: https://github.com/candlepin/candlepin-jobs/. Vagrant can be used for testing Jenkins jobs, but our Vagrant machines are used for Jenkins CI. Vagrant machines are used for testing mostly be developers.

The Cockpit CI integration here has the downside that it only tests the new Cockpit code; it doesn't build and install a new rhsmd, for example. I think that's a significant drawback, and if going with Jenkins would fix that, that would be a strong argument.

Yeah, when Vagrant machine is used, then rhsmd and everything else is just as fresh as possible.

I have added a commit here that installs rhsm into our VM image.

jirihnidek · 2019-06-11T07:34:34Z

test/check-subscriptions

+        args = {"addr": "10.111.113.5"}
+        m.execute(script=WAIT_SCRIPT % args)
+
+    def testRegister(self):


I can see several tests in this function. Could it be refactored little bit? Could you split it into several functions?

The overhead to setup a test is significant, unfortunately, so we tend to bunch a couple of tests together into a single run just for performances sake... But I see your point, of course. I'll split them up.

The overhead to setup a test is significant, unfortunately, so we tend to bunch a couple of tests together into a single run just for performances sake... But I see your point, of course. I'll split them up.

You needn't to use setUp() for starting tomcat. You can also use setUpClass() for this purpose. https://docs.python.org/3/library/unittest.html#unittest.TestCase.setUpClass

You can also use setUpClass() for this purpose.

Ohh, nice! I'll use that.

You can also use setUpClass() for this purpose.

Ohh, nice! I'll use that.

That didn't work out as expected. The serUpClass method can't start any machines since that needs an instance. (I guess we can try adding support for "infrastructure" machines that are meant to be shared between tests.)

jirihnidek · 2019-06-11T07:38:06Z

test/check-subscriptions

+        sleep 1
+    fi
+done
+"""


Well. I guess it can work, but I would like to see usage of subprocess and e.g. request module. Calling bash script from python script is unnecessary hack :-)

You can see example of using requests and candlepin API e.g. here: https://github.com/candlepin/candlepin/blob/master/server/bin/create_test_repos.py#L258

Calling bash script from python script is unnecessary hack :-)

This script runs in the VM itself, via self.machine.execute(...), so this is not such a big hack as it might seem.

Still, Python is strictly nicer than bash, so I'll rewrite the bash bits.

jirihnidek · 2019-06-11T07:43:55Z

test/check-subscriptions

+data = json.loads(sys.stdin.read())
+print [e['id'] for e in data if e['name'] == 'awesome_os_pool' and e['owner']['displayName'] == 'Admin Owner'][0]
+"
+"""


Again. Please use requests module. This is too big hack (from Python call bash, then call curl, then parse output of curl using Python).

Done. Generating the key can be done in a single step, so I have combined these two scripts.

jirihnidek · 2019-06-11T08:22:12Z

test/check-subscriptions

+data = json.loads(sys.stdin.read())
+print [ e['id'] for e in data if e['owner']['key'] == 'admin' and e['contractNumber'] == '0' and [p for p in e['providedProducts'] if p['productId'] == '88888'] ][0]
+"
+"""


Same request here.

jirihnidek · 2019-06-11T08:26:19Z

test/check-subscriptions

+
+class TestSubscriptions(MachineCase):
+    provision = {
+        "0": {"address": "10.111.113.1/20"},


Is it always this IP address? If yes, then please use some constant for that. if no, then it should be possible specify it using some configuration file or command line argument.

Yes, good point.

jirihnidek · 2019-06-11T08:26:36Z

test/check-subscriptions

+class TestSubscriptions(MachineCase):
+    provision = {
+        "0": {"address": "10.111.113.1/20"},
+        "candlepin": {"image": "candlepin", "address": "10.111.113.5/20"}


Same comment here for this IP address.

jirihnidek · 2019-06-11T08:42:53Z

test/check-subscriptions

+        self.candlepin.execute("systemctl start tomcat")
+
+        # download product info from the candlepin machine
+        product_file_one = os.path.join(self.tmpdir, "6050.pem")


I recommend to create some dictionaries with definition of products. Something like this:

PROD_SNOWY = { "id": "6050", "name": "Snowy OS Premium Architecture Bits" }

The name of product can be changed in the future a little and it will be easier to fix it. The name of 88888 in master branch is now: Shared File System Bits (no content).

mvollmer · 2019-06-11T10:40:51Z

Does it include bug fix (it seem so)?

Yes, there are five commits with bug fixes. Do you want five PRs and bug reports for them?

jirihnidek · 2019-06-11T13:07:24Z

Does it include bug fix (it seem so)?

Yes, there are five commits with bug fixes. Do you want five PRs and bug reports for them?

It would be great (our QE team would love it 😃 ), but one bug report with one PR could be IMHO acceptable too.

mvollmer · 2019-06-12T12:37:01Z

Does it include bug fix (it seem so)?

Yes, there are five commits with bug fixes. Do you want five PRs and bug reports for them?

It would be great (our QE team would love it smiley ), but one bug report with one PR could be IMHO acceptable too.

Alright, I made four bug reports and four PRs: #2116, #2114, #2113, #2112. :-)

jirihnidek · 2019-06-12T15:15:48Z

@mvollmer You are the best! 😃

mvollmer · 2019-06-14T11:07:49Z

The "Don't concurrently set config and attach" fix is argueably a workaround for a bug in rhsm. It should just cope with overlapping Config.Set calls. I think I'll open an issue about that.

Just to tie it all up: Bug and fix.

mvollmer · 2019-07-24T12:46:37Z

@jirihnidek, can we move this forward?

jirihnidek

Hi Marius, thanks for your updates. 👍 I have few requests.

jirihnidek · 2019-07-31T09:39:16Z

.gitignore

+/bots
+/cockpit/subscription-manager-cockpit.tar.gz
+/node_modules
+/test


Directory test should not be in .gitignore, because it includes files with unit tests. I wonder, why do you use absolute path starting with /?

The leading / is there so that only the top-level test matches. Just test would also match ./syspurpose/test, for example.

/test specifically is in .gitignore because running the tests will create at least ./test/common and ./test/images, but I agree that ignoring all of ./test is wrong.

I'll move all new files out of ./test so we also don't need to ignore anything in there.

jirihnidek · 2019-07-31T09:41:51Z

test/check-subscriptions

@@ -0,0 +1,248 @@
+#!/usr/bin/python3


Directory ./test/ contains only unit tests at this moment. Please could you move it to some subdirectory (e.g. integration_tests)?

Yes, I was waiting for this. :-) I'll move the files away. This might require some changes in the Cockpit CI code, but that's for the benefit of everyone so shouldn't be an issue.

jirihnidek · 2019-07-31T09:42:53Z

test/check-subscriptions

+        args = {"addr": "10.111.113.5"}
+        m.execute(script=WAIT_SCRIPT % args)
+
+    def testRegister(self):


jirihnidek · 2019-07-31T09:44:31Z

test/run

@@ -0,0 +1,77 @@
+#! /usr/bin/make -f


Please could you move this file to some subdirectory too?

jirihnidek · 2019-07-31T09:46:01Z

test/vm.install

@@ -0,0 +1,29 @@
+#!/bin/sh


Please move this file to some subdirectory too.

This also brings the necessary infrastructure.

mvollmer · 2019-08-01T11:44:04Z

@jirihnidek, I have moved all the new files into the new directory ./integration-tests.

The entry point for the Cockpit bots is now .cockpit-ci/run. You can see a demo in the mvollmer#1 PR, as earlier.

mvollmer · 2019-08-01T11:46:42Z

@jirihnidek, forgot to mention: The Cockpit bots still put some files into test/images while they do their work. They will stop doing that at some point, but I can't yet say when exactly.

jirihnidek

ACK

mvollmer · 2019-08-01T13:13:59Z

Woohoo! Thanks!

mvollmer force-pushed the cockpit-ci-tests branch from 1d32f8a to 277fac4 Compare May 28, 2019 12:33

mvollmer changed the title ~~test: Add check-subscriptions to test the Cockpit UI~~ test: Add check-subscriptions to test the Cockpit UI and make it pass May 28, 2019

mvollmer force-pushed the cockpit-ci-tests branch 3 times, most recently from 2e77062 to 966e537 Compare May 29, 2019 13:08

mvollmer force-pushed the cockpit-ci-tests branch 2 times, most recently from 50f14ca to e6d75f3 Compare June 5, 2019 08:02

jirihnidek self-assigned this Jun 10, 2019

jirihnidek requested changes Jun 11, 2019

View reviewed changes

mvollmer force-pushed the cockpit-ci-tests branch from e6d75f3 to c04b712 Compare June 12, 2019 12:37

mvollmer changed the title ~~test: Add check-subscriptions to test the Cockpit UI and make it pass~~ test: Add check-subscriptions to test the Cockpit UI Jun 12, 2019

mvollmer force-pushed the cockpit-ci-tests branch 3 times, most recently from 6f1eed8 to 0dc4748 Compare June 17, 2019 11:57

mvollmer force-pushed the cockpit-ci-tests branch from 0dc4748 to 20a9401 Compare June 26, 2019 09:24

jirihnidek requested changes Jul 31, 2019

View reviewed changes

mvollmer force-pushed the cockpit-ci-tests branch 2 times, most recently from 074b327 to 6cfdd30 Compare July 31, 2019 13:33

test: Add check-subscriptions to test the Cockpit UI

23e6c12

This also brings the necessary infrastructure.

mvollmer force-pushed the cockpit-ci-tests branch from 6cfdd30 to 23e6c12 Compare August 1, 2019 11:17

jirihnidek approved these changes Aug 1, 2019

View reviewed changes

jirihnidek merged commit 028ece3 into candlepin:master Aug 1, 2019

ptoscano mentioned this pull request Mar 1, 2022

cockpit: set RPM_BUILD_ROOT w/ installing #2989

Merged

Conversation

mvollmer commented May 24, 2019

Uh oh!

mvollmer commented May 24, 2019

Uh oh!

candlepin-bot commented May 24, 2019

Uh oh!

mvollmer commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvollmer commented May 24, 2019

Uh oh!

jirihnidek commented May 27, 2019

Uh oh!

jirihnidek commented May 27, 2019

Uh oh!

mvollmer commented May 27, 2019

Uh oh!

mvollmer commented May 27, 2019

Uh oh!

mvollmer commented May 28, 2019

Uh oh!

mvollmer commented May 29, 2019

Uh oh!

mvollmer commented Jun 5, 2019

Uh oh!

jstavel commented Jun 7, 2019

Uh oh!

jirihnidek commented Jun 10, 2019

Uh oh!

mvollmer commented Jun 10, 2019

Uh oh!

jirihnidek commented Jun 10, 2019

Uh oh!

jirihnidek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mvollmer commented May 24, 2019 •

edited

Loading