Feature Automatically update PRs before running checks

MCroft

Developer
Staff member
Anyways, build is green again as of the latest commit.
Still can't quite figure out why PR 123 (which is a button added to a panel) is causing the AshInterop test to fail.

Is there any way to run all three OS tests regardless of the others failing? At least until we get done with platform-specific failures?
 

heeheehee

Developer
Staff member
From a previous stint of debugging, I determined that JavascriptRuntime.execute() will return null if Mafia is in an error state when it runs.

That PR is 20 commits behind origin/main. I recommend merging in the latest changes to pick up the improvements to test robustness.

I don't know enough about github workflows to make all the tests always run, but I suspect it would be more complicated than it's worth.
 

fronobulax

Developer
Staff member
Still can't quite figure out why PR 123 (which is a button added to a panel) is causing the AshInterop test to fail.

Those tests passed locally on Windows, remotely on GitHub for all OSes and then failed while running coverage. I have to believe there is test leakage and perhaps the test and jacocoTestReport tasks have some subtle difference in their environments? They just passed locally again.

"That PR is 20 commits behind origin/main" doesn't quite make sense to me unless my SVN habit of upgrading to the latest and greatest and "testing" before a commit is not required or always done in GitLand. Perhaps it should be until unrelated tests stop breaking?

In other news Microsoft wants me to adopt Windows 11 so we may need to add one more OS to the GitHub checks. But I'm not going to 11 yet :)

 

heeheehee

Developer
Staff member
"That PR is 20 commits behind origin/main" doesn't quite make sense to me unless my SVN habit of upgrading to the latest and greatest and "testing" before a commit is not required or always done in GitLand. Perhaps it should be until unrelated tests stop breaking?
So, I don't know if you're familiar with branches in SVN, but it's very loosely analogous.

You may hear that git is a tree (technically incorrect in the presence of merges), or a directed acyclic graph (if you want to get technical about it), where the edges correspond to commit ancestry.

Traditionally in git, branches are not special. By convention, master (main in github) is the default branch when creating a new repository, but you don't need to keep it around if you would like it to be called "fronos-awesome-branch" instead. Commits are not numbered, but rather just know about their parent(s). (A commit can have multiple parents if it's the result of a merge.)

When you create a new branch, you declare a certain commit to be the HEAD of that branch. You can then add descendant commits, which will show up in that branch, or merge the histories of two (or more) branches. But there's no requirement to merge in your main branch (since main isn't special -- you could delete it if you wanted), much less the main branch of a specific remote repository (e.g. origin/main).

(Note that "origin" is also not a special remote name nor is it required by git. It's the default remote name if you clone a repository without giving it an explicit name.)

I'll present a concrete example. This branch says: "This branch is 3 commits ahead, 23 commits behind kolmafia:main."

What that means is that the branch contains 3 commits not in kolmafia:main (2f64b4b, afcefca, 5c8de80), and the last common ancestor (4fe6139, as added by a merge, which you'll note has two parents) was 23 commits prior to the current head of kolmafia:main as of when I checked just now.

(I don't know if that helps clear things up, but I hope it does.)
 

MCroft

Developer
Staff member
@heeheehee, I do get the branch model, but I guess I expect that Pull request to be evaluated against the HEAD of the branch it's pulled into, like an SVN merge. I expect that if it's blocked because it's a non-trivial merge, it will tell me in the PR (I have seen this). It seems like it's definitely good practice to update and re-push if something needs to be changed, but it also seems like a fragile process.

what am I missing?
 

fronobulax

Developer
Staff member
I kind of get it but it seems like we are doing something wrong when unrelated tests fail and a fix is on a branch that could have been pulled in but wasn't. Teething problems :)
 

heeheehee

Developer
Staff member
How do you envision this working?

Would we want our github action to locally merge origin/main into the branch (e.g. git merge --no-edit origin/main)? What should it do if it's a nontrivial merge? Fail and say "hey you need to merge"? Should we just fail if it's behind origin/main at all?
 

fronobulax

Developer
Staff member
How do you envision this working?

Would we want our github action to locally merge origin/main into the branch (e.g. git merge --no-edit origin/main)? What should it do if it's a nontrivial merge? Fail and say "hey you need to merge"? Should we just fail if it's behind origin/main at all?

I don't know what makes sense in a GitHub world.

The SVN model is no branches and the well trained dev has a responsibility to update to the latest, resolve all merges and do dev testing before commit. Of course the people who didn't like that are the people who invented Git so my expectations are what need to be adjusted.
 

MCroft

Developer
Staff member
When I generated a merge conflict, I got this message. This seems like a good thing.
merge_conflict_error_on_github.png

If it can trivially merge, I'd expect the PR to do so when I run the test. It shouldn't just test the PR as it came over 15 days and 10 revisions ago. I think it does, but I can't tell if it does that trivial merge for every test re-execution or if what gets tested is different from what the end product of the commit will be.

If HEAD changes for the receiving branch, it might block merging, but if the changes aren't relevant (e.g. a new file), maybe it doesn't. I'd expect GitHub to use the VCS to take care of it.

I may really not get how this works, so sorry about that.
 

fronobulax

Developer
Staff member
I don't like it when something fails on a PR I submitted and it is Not My Fault!!!

If there were a way to take any PR, simulate merging it with the latest version of main and then run the tests that would make me happier. A failure would be mine to fix, perhaps some time in the future, and if doing so eliminates a failure then I know the problem has been fixed and can ignore the failure.

That said as we squash test failures that are due to leakage or just a not quite correctly written test this will become less of a concern.
 

xKiv

Active member
In a git world, aren't you *supposed* to merge [1] your upstream into your feature branch before you you make a PR?
(in a more centralized environment, I am still responsible for pushing only completely resolved merges)

[1] or rebase, depending on the project's culture, your sensibilities, how willing you are to deal with merge conflicts on a commit-by-commit basis rather than once at the end, and I don't even know what else
 

heeheehee

Developer
Staff member
Supposed to? Aspirationally, yes. With a large codebase that has frequent commits, that's not as feasible as the less stringent requirement "can be merged cleanly." In particular, if the PR process takes a long time, then there's potential for upstream fixes to land (as they did this time) between the time that the PR was created (at which point it was presumably up-to-date) and when the PR is accepted + merged.

If I'm reviewing a PR, I don't need to see "Merge main into feature-branch" as 90% of the commits, especially if the upstream changes are entirely unrelated.
 

Ryo_Sangnoir

Developer
Staff member
From my understanding of GitHub Actions, using the `pull_request` target should do what you want -- the resulting action should run in the context of the merge commit, not the context of the branch. `pull_request_target` does the latter.

Now, you already have it as `pull_request`. Why do you think it's not working?
 

fronobulax

Developer
Staff member
Since I started this...

The biggest problem is when I have an approved PR and I merge it. The GitHub workflow then runs tests (that had previously passed everywhere) and one or more tests fails. Can the workflow automatically do something to detect this?

In a SVN world without branching this only happened when through developer neglect or unlucky timing the remote repo changed between the developer's last local update and the commit.

This became an issue because our understanding of how to write good tests for KoLmafia is evolving. At one point we had tests that succeeded or failed depending upon what other tests had been run or in what order. So detecting that early could be beneficial and possibly indicate tests in the PR were deficient.

But since the root cause in the only specific examples I can recall was leaking tests, not developer behaviour, this problem may not need to be solved because we are learning how to make less leaky tests.
 

heeheehee

Developer
Staff member
From my understanding of GitHub Actions, using the `pull_request` target should do what you want -- the resulting action should run in the context of the merge commit, not the context of the branch. `pull_request_target` does the latter.

Now, you already have it as `pull_request`. Why do you think it's not working?
Hm. That was in the context of https://github.com/kolmafia/kolmafia/pull/123#issuecomment-948274307, although upon further inspection, it seems that the corresponding run was a few hours prior to the fix in question.

But, I tried rerunning all tasks just now, and I'm still seeing test failures.
 

xKiv

Active member
If I'm reviewing a PR, I don't need to see "Merge main into feature-branch" as 90% of the commits, especially if the upstream changes are entirely unrelated.
That points more to "do rebases" instead of "don't make sure that the PR is against current code", no?
 

xKiv

Active member
If you're willing to accept force pushes, sure. Rebases rewrite history.
Hmm .. guess I have never tried rebasing something that hasn't been pushed yet, then. (my experience is doing/committing some work locally, failing a push, failing a pull --ff-only, then pull --rebase ; I don't wan't to create "merge origin/abc into abc" commits on something that nobody else has seen yet)
 
Top