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

configure PAPR to run tests in PRs, not just 'improved-sanity-test' #171

Closed
miabbott opened this issue Jun 6, 2017 · 5 comments
Closed

Comments

@miabbott
Copy link
Collaborator

miabbott commented Jun 6, 2017

Our PAPR implementation is pretty rudimentary - it will run the improved-sanity-test on multiple platforms, which is a start. But often new PRs are introducing new roles/tests that aren't covered by the improved-sanity-test.

I'm thinking we need a helper script to determine if a test has been changed or added, then run that test.

This might be a little tricky if the PR just modifies a role (although, I guess you could grep to see which test used said role), but we can always fallback to just running improved-sanity-test

Tagging @jlebon because I feel like he may have helped solve this problem elsewhere.

@jlebon
Copy link
Contributor

jlebon commented Jun 7, 2017

Right, definitely doable. You can use e.g. git diff --name-only to get just the list of filenames modified in the PR, and then apply some heuristics on that. There's an upstream issue open in PAPR to support this natively: https://github.com/jlebon/papr/issues/27.

The closest thing we have right that's "self-aware" is the ostree/rpm-ostree submodule checker: https://github.com/projectatomic/rpm-ostree/blob/8b8bdcc/ci/ci-commitmessage-submodules.sh. One tricky part worth noting is that it uses the PAPR-injected env var PAPR_COMMIT to make sure we're not doing our diffs based on the merge commit, but from the PR HEAD itself.

@miabbott
Copy link
Collaborator Author

miabbott commented Jun 8, 2017

@mike-nguyen
Copy link
Collaborator

@jlebon Maybe its a difference in workflow but whenever I do a git diff on origin/master..$PAPR_COMMIT, it picks up files that have been merged since the PR was originally submitted. The PRs are usually built on top of older commits so they are usually behind HEAD. Do you usually rebase before pushing a new PR?

From looking quickly at the PAPR code, I think the diff I actually want is between the PR merge and master i.e. git diff origin/master..PAPR_MERGE_COMMIT. That should give me only the files I have changed on top of master. WDYT? My git-fu is novice level so please feel to correct me!

@jlebon
Copy link
Contributor

jlebon commented Jun 14, 2017

Ahh, OK this is interesting. Ordinarily, A..B means "all commits reachable from B but not from A", which is what we want here (see https://git-scm.com/docs/gitrevisions#gitrevisions-Theememtwo-dotRangeNotation). However, git diff interprets them differently than other commands (see the last paragraph of DESCRIPTION here: https://git-scm.com/docs/git-diff).

So I think what we want here is instead something like: git diff $(git merge-base origin/master HEAD).

@mike-nguyen
Copy link
Collaborator

So I think what we want here is instead something like: git diff $(git merge-base origin/master HEAD)

@jlebon That seems to work in the scenarios I've tested 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants