Skip to content
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

tests-scan: re-enable cross-project 'job' creation #6066

Merged
merged 18 commits into from
Mar 12, 2024
Merged

Conversation

allisonkarlitskaya
Copy link
Member

@allisonkarlitskaya allisonkarlitskaya commented Mar 11, 2024

This should be mostly right. I'm not 100% sure about this part, though:

+                    "branch": job.ref,

lib/aio/github.py Fixed Show resolved Hide resolved
@martinpitt
Copy link
Member

Aside from blocking on the integration test cockpit-project/cockpituous#593 , I'd like to add a unit test here. Covering a cross-project test in the unit test should be quite simple, I'm happy to look at this tomorrow.

@allisonkarlitskaya
Copy link
Member Author

Landed the cockpituous changes and retriggered the test run here.

@allisonkarlitskaya
Copy link
Member Author

aiohttp.client_exceptions.ClientResponseError: 404, message='Mock Not Found: /repos/cockpit-project/cockpituous/git/refs/heads/main', url=URL('http://127.0.0.7:8443/repos/cockpit-project/cockpituous/git/refs/heads/main')
ERROR:root:./job-runner json {"repo": "cockpit-project/bots", "sha": "f54c3bcd3cf209190483080da455b636e780550e", "context": "unit-tests@cockpit-project/cockpituous", "target": "main", "pull": 1, "report": null, "command-subject": {"repo": "cockpit-project/cockpituous", "branch": "main"}, "slug": "pull-1-f54c3bcd-20240311-183244-unit-tests", "env": {"TEST_OS": "unit-tests", "COCKPIT_BOTS_REF": "f54c3bcd3cf209190483080da455b636e780550e", "BASE_BRANCH": "main"}, "container": "quay.io/cockpit/tasks", "secrets": ["github-token", "image-download"]} failed with exit code 1

That's one for the mocking framework in cockpituous, and I guess it's not going to be super trivial to fix it, either...

I start to think about reviving my git-serve mock thingy....

@martinpitt
Copy link
Member

I'll send a test fix to provide the missing gh API endpoint in a minute. In the meantime, the test fails like this now:
The cross-project test run creates a slug "pull-1-f54c3bcd-20240312-070616-unit-tests", but the test expects an additional "-cockpit-project-cockpituous" suffix to disambiguate it. I think we should fix that here. That'll also be covered in the unit test (getting to that next).

martinpitt added a commit to cockpit-project/cockpituous that referenced this pull request Mar 12, 2024
job-runner needs that for cross-project testing [1]. A clean way would
be to actually resolve "main" to a current SHA in the test, but as
nothing actually cares, just rely on the `main` fallback for now --
nothing cares whether this is an actual SHA, as most things in git are
happy with a "ref" -- and we don't use that to post statuses (and that
would fail because the mock doesn't provide these paths).

[1] cockpit-project/bots#6066
allisonkarlitskaya pushed a commit to cockpit-project/cockpituous that referenced this pull request Mar 12, 2024
job-runner needs that for cross-project testing [1]. A clean way would
be to actually resolve "main" to a current SHA in the test, but as
nothing actually cares, just rely on the `main` fallback for now --
nothing cares whether this is an actual SHA, as most things in git are
happy with a "ref" -- and we don't use that to post statuses (and that
would fail because the mock doesn't provide these paths).

[1] cockpit-project/bots#6066
@martinpitt
Copy link
Member

@allisonkarlitskaya I added a unit test, adjusted it for re-enabling job (the FIXUP), and added two more commits for the fallout it detected. So while I rebased, I didn't touch your commits, all my changes are the ones authored by me -- for easier review. Obviously we should squash the fixup before we land.

First test run to see where I screwed up, i.e. where the unit and integration tests disagree.

martinpitt
martinpitt previously approved these changes Mar 12, 2024
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

🟢 🎉 I'm good with your changes. Please review mine, then you or me can do the squashing and we send this productionwards

Copy link
Member Author

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

I can't approve (own PR).

One ask: can we also have a test for checking cross-project runs on the non-default branch? I really want to make sure that the logic is right there — it's the biggest part of this PR that I'm unsure about (mostly because of the impenetrable logic of tests-scan)

# mock time for predictable test name
@unittest.mock.patch("time.strftime", return_value="20240102-030405")
@unittest.mock.patch("task.distributed_queue.DistributedQueue")
def test_amqp_sha_pr_cross_project(self, mock_queue, _mock_strftime):
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if I could convince you to add this as a pytest-style test. We're going to want to convert everything at some point anyway...

You're probably interested in the monkeypatch fixture, if you do that.

@martinpitt
Copy link
Member

test for checking cross-project runs on the non-default branch?

Good call -- I think that's wrong right now, it sets job.target to the test's branch, but it's supposed to be the target branch of the PR.

Make this a bit easier to read and tweak how we handle JSON formatting.
Instead of special-casing the top-level, add a `default` handler which
encodes all encountered objects via `__dict__`.  We're going to
introduce a new object soon.
We may also (some day) split this into a sub-object in the "job" object,
as it appears on the wire, but for now let's not break protocol.
This is the same as the fields which allow us to specify what we've been
calling "subject" up to this point, but refers exclusively to the thing
that we checkout and run.

The statuses (and any issues or anything else) will continue to be
reported against the main Subject, as before.

We copy our existing get_object() code from Cockpit to help us with
this.

This enables us to run cross-project tests, albeit still in an ad-hoc
manner (but no worse than we had it before).
pyright noticed that we were making use of an uninitialized variable in
this case.  Make sure we return if the file is missing.
Before, if we got a sha given as part of the Job, we'd assume that we
also got the target given to us and skip looking up anything on the PR
(when it was set).

Change that logic a bit: now, if the pull number is given, always do the
lookup and use it as default values for both the sha and the target.  It
should almost never be necessary to specify the target from outside,
anymore.
clone_url is a really a derived property of the repo and the forge.
Store those in there, instead, and turn clone_url into a property.
We do the same thing for GET and POST, so re-use that code.  We're about
to reuse it some more, and a third copy is really just too many.
allisonkarlitskaya and others added 9 commits March 12, 2024 12:50
Add support for fetching file content from GitHub via the
raw.githubusercontent.com interface.
In case the job didn't explicitly specify a container image to use,
query the target repository for the `.cockpit-ci/container` file.  Only
fall back to the default container image if we get a 404.

Also: add a message to the log about which container image we decided to
use in the end.
job-runner is interested in the thing we call `github_context` here, so
make sure we use it instead.

Fixes #6065
We can do this inline, avoiding some messy conditionals.

Adjust tests and use normal asserts in a couple places.
This validates the mechanics for make-checkout/tests-invoke and confirms
commit c377eb8. Cover both "implicit default branch" and "explicitly
specified branch" scenarios.
We're actually interested in options.repo, not job.repo.  Job.repo is
what gets called "context_project" elsewhere, and refers more to the
command that we want to run than it does to the thing we're interested
in testing (and definitely doesn't refer to the thing we report status
on).

This change doesn't actually do anything (since we currently ignore the
case where job.repo != options.repo) but it's important that we get the
semantics correct here, because it's about to start mattering.

job.repo will be handled differently in the next commit.
If the test project isn't the same as the status repo (i.e. a
cross-project test), then we need to read the container file from the
actual test repo.

This isn't yet covered by unit tests due to commit c377eb8, but the
next commit will enable that again and check it.
We want to include the test repository for cross-project tests, which is
`job.github_context`.
It's going to look this up on its own, now.
allisonkarlitskaya and others added 2 commits March 12, 2024 14:22
job-runner can do this on its own now.
Use the new 'command-subject' support in job-runner to bring back
cross-project support for creating 'Job' objects.

Co-authored-by: Martin Pitt <[email protected]>
@martinpitt
Copy link
Member

In your previous update, you accidentally squashed two commits together. I broke e7f0c9f ("tests-scan: re-enable cross-project 'job' creation") out again, and fixed the trivial ruff failure. I also ensured that test/run passes in the intermediate commits now.

The net diff is just the ruff fix now.

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

CI and me are happy! 🚀

@martinpitt martinpitt marked this pull request as ready for review March 12, 2024 13:34
@martinpitt martinpitt merged commit 3f0c87c into main Mar 12, 2024
6 checks passed
@martinpitt martinpitt deleted the cross-jobs branch March 12, 2024 13:36
@martinpitt
Copy link
Member

This was referenced Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants