Normalize tests invocation #12367

marusak · 2019-07-19T10:53:11Z

I am just testing this. And with having opened PR that has these changes (when this lands, all PRs needs to rebase) it is easier to test.

Blocked on:

martinpitt · 2019-07-19T11:14:47Z

bots/tests-invoke

@@ -183,7 +181,7 @@ class PullTask(object):
            # explicit request for html logs
            status["link"] = "log.html"
            status["extras"] = [ "https://raw.githubusercontent.com/cockpit-project/cockpit/master/bots/task/log.html" ]
-        elif self.context.startswith("cockpit/"):
+        elif self.test_project:


This will regress the log.html bits for cockpit further down below, right? At least we want test_project to be something like cockpit-project/cockpit for cockpit itself, or isn't that the case yet?

Right, I also though that TEST_PROJECT would be set up to cockpit-project/cockpit but it isn't.
This is what tests-scan outputs for cockpit test:

PRIORITY=0001 bots/make-checkout --verbose --base=master --repo=cockpit-project/cockpit pull/12361/head 0985a3bbfff2a97fa7f36d1a9253280db9dd3202 && GITHUB_BASE=cockpit-project/cockpit TEST_NAME=pull-12361-20190719-132222 TEST_REVISION=0985a3bbfff2a97fa7f36d1a9253280db9dd3202 bots/tests-invoke --pull-number=12361 --rebase=master fedora-30/selenium-chrome pull/12361/head

and this is for external project:

PRIORITY=0005 bots/make-checkout --verbose --base=master --repo=cockpit-project/starter-kit --bots-ref=pull/12361/head '' && TEST_PROJECT=cockpit-project/starter-kit TEST_NAME=pull-12361-20190719-132222 TEST_REVISION=0985a3bbfff2a97fa7f36d1a9253280db9dd3202 bots/tests-invoke --pull-number=12361 --rebase=master --remote=test centos-7 ''

I admit that I took this shortcut and just misused the fact that this is not set up.

Right, I figured something like that -- too many "cockpit" special cases. While we are at it, could you please test for cockpit here, and somewhere at the top make it default to cockpit, with a FIXME?

test/run

marusak · 2019-07-21T05:38:38Z

bots/image-refresh

+        "fedora-30/selenium-chrome",
+        "fedora-30/selenium-firefox",
+        "fedora-30/selenium-edge",
+        "fedora-30/avocado-fedora",


This should be just fedora-30/avocado-fedora, right? the -fedora is pointless there.

Right, let's make this just /avocado (I suppose that's what you actually meant).

marusak · 2019-07-21T16:03:21Z

bots/task/github.py

@@ -55,6 +55,7 @@
 # it will publish a test task to the queue (used to trigger specific contexts)
 NOT_TESTED_DIRECT = "Not yet tested (direct trigger)"

+# TODO this is not gonna work, is it?


Now we won't be able to use this OUR_CONTEXTS, since tests are going to start always with TEST_OS. @martinpitt mentioned something about this and that he talked about it with @Gundersanne. What was the agreed solution?

I had something else in mind when I said that we shouldn't need a static "cockpit/" prefix. Indeed we'll have to rework this. known_context() is used in tests-trigger as a sanity check to prevent us trying to e. g. retry semaphore with this (which won't work). But the more important user is statuses().

So either known_context() checks if the first part (.split('/')[0]) exists (as link) in bots/images/ (some caching should be involved), or we have to use a static cockpit/ prefix. I'm just a bit concerned about test names getting too long, with the extra /branch name that we want to tack at the end. We could shorten it to "c/"..

But WDYT about the "is this a known image" check? It's not that expensive and easy to do.

But WDYT about the "is this a known image" check? It's not that expensive and easy to do.

I agree that checking if the first part is valid image makes sense. Adding cockpit could make some names way too long and it is still just noise. The same goes with c/. I'll take a look at this and send some commit.

martinpitt · 2019-07-22T10:05:33Z

Both prerequisite PRs landed, unblocking.

marusak · 2019-07-23T05:20:25Z

bots/task/github.py

+    for project in testmap.projects():
+        for branch_tests in testmap.tests_for_project(project).values():
+            if context in branch_tests:
+                return True


One problem with this - this will mark fedora-30/verify as unknown context although it is equivalent to fedora-30 which would be a known context. Is this a problem and should this just split context at \ and then split also all contexts from the map and check it exists?

It's a bit unfortunate, but I don't think it will be a real-life problem. In principle we have this problem today already with non-cockpit problems. I think it's fine to start with the convention that the "verify" scenario isn't spelled out.

marusak · 2019-07-23T05:22:38Z

bots/task/testmap.py

        ],
-        'rhel-7.7': ['avocado/fedora', 'container/kubernetes', 'container/bastion',
-            'selenium/firefox', 'selenium/chrome', 'verify/rhel-7-7',
+        'rhel-7.7': ['fedora-30/avocado-fedora', 'fedora-30/container-kubernetes',


containers, avocado and selenium are always F30 no matter which branch, right?

I'd rather not make this assumption. We regularly update the image for these, and then also (have to) update the stable branches to keep up. But the point of this is to be more flexible, and always specify the OS. What's the background of your question?

marusak · 2019-07-23T05:23:24Z

Rebased and edited to work with new changes. I've posted two comments in the code that I am not certain about.

All projects support `test/run` script, so lets use this. Also context now is image[/scenario]. Scenario cannot contain '/' and is not necessary. It is every `test/run` scripts duty to be able to parse TEST_OS and SCENARIO environment variables.

martinpitt

This looks fine now, thank you! Rolling this out will cause some disturbance on existing PRs as you can't retry failed tests with "old" names any more. Once this lands, I'm happy to ping @atodorov and other affected people, but please send out a quick mail to cockpit-devel@ about this (even before landing).

Asking @Gundersanne to have a final look, as she also worked on this. Otherwise, please land when ready, and we stand by to do emergency actions if something fails :-)

croissanne

Compared it to whatever I had, it seems to do all of that and more, and more elegantly!

I also tried this out a little bit with a local amqp instance, it seems to produce the correct results (though running the commands, it fails at tests-invoke in the rebase which is normal as all current PR's will need a rebase).

so LGTM! This will make stuff a lot nicer :D

marusak added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jul 19, 2019

marusak force-pushed the unify_test_names branch 2 times, most recently from 1ad2112 to 8ab20f5 Compare July 19, 2019 11:15

martinpitt reviewed Jul 19, 2019

View reviewed changes

martinpitt requested changes Jul 19, 2019

View reviewed changes

marusak force-pushed the unify_test_names branch from 8ab20f5 to 2badc91 Compare July 19, 2019 13:56

marusak added the blocked Don't land until something else happens first (see task list) label Jul 19, 2019

marusak mentioned this pull request Jul 19, 2019

tests: Introduce test/run script for Cockpit #12368

Merged

marusak commented Jul 21, 2019

View reviewed changes

marusak force-pushed the unify_test_names branch from 2badc91 to ba66979 Compare July 21, 2019 15:59

cockpituous added the test-external label Jul 21, 2019

marusak commented Jul 21, 2019

View reviewed changes

martinpitt removed the blocked Don't land until something else happens first (see task list) label Jul 22, 2019

marusak force-pushed the unify_test_names branch 2 times, most recently from 7720777 to f8ebb22 Compare July 23, 2019 05:18

marusak commented Jul 23, 2019

View reviewed changes

bots: Normalize tests invocation

78336d3

All projects support `test/run` script, so lets use this. Also context now is image[/scenario]. Scenario cannot contain '/' and is not necessary. It is every `test/run` scripts duty to be able to parse TEST_OS and SCENARIO environment variables.

marusak force-pushed the unify_test_names branch from f8ebb22 to 78336d3 Compare July 23, 2019 05:26

martinpitt approved these changes Jul 23, 2019

View reviewed changes

martinpitt requested a review from croissanne July 23, 2019 06:45

croissanne approved these changes Jul 23, 2019

View reviewed changes

marusak merged commit 2cf6e3a into cockpit-project:master Jul 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize tests invocation #12367

Normalize tests invocation #12367

marusak commented Jul 19, 2019 •

edited by martinpitt

Loading

martinpitt Jul 19, 2019

marusak Jul 19, 2019

martinpitt Jul 19, 2019

marusak Jul 21, 2019

martinpitt Jul 21, 2019

marusak Jul 21, 2019

martinpitt Jul 22, 2019

marusak Jul 22, 2019

martinpitt commented Jul 22, 2019

marusak Jul 23, 2019

martinpitt Jul 23, 2019

marusak Jul 23, 2019

martinpitt Jul 23, 2019

marusak commented Jul 23, 2019

martinpitt left a comment

croissanne left a comment

Normalize tests invocation #12367

Normalize tests invocation #12367

Conversation

marusak commented Jul 19, 2019 • edited by martinpitt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marusak commented Jul 23, 2019

martinpitt left a comment

Choose a reason for hiding this comment

croissanne left a comment

Choose a reason for hiding this comment

marusak commented Jul 19, 2019 •

edited by martinpitt

Loading