simplify testing by removing unnecessary reference

changed milestone to %First proper release

If I understand this right, if a submitter is running the tests themselves, the results will be different before vs after they commit their work. After they commit they need to specify a previous reference. Will that confuse people? I wonder if it's possible to have it test by default against origin/master if that exists.

Yes, that's correct. The reference is 'HEAD', will always be the latest HEAD of the current branch, which will be updated on every commit. We could instead set the reference to be 'origin/master', or just 'master', but there's no guarantee that the user is using the name 'origin' and 'master' in the canonical way. It's easy to compare against any reference, though, with the '--git-ref' option, i.e.:

$ python3 -m gwinc.test --git-ref origin/master

I no longer think the previous way of storing a reference to a particular git hash will work. Since we allow merge commits, we could easily end up with a situation where we have conflicts on the reference file that would be tricky and annoying to resolve.

I guess I'm just saying, if the command you quoted above is what most people would want most of the time, why not make it the default?

If someone has changed the definitions of origin and/or master, they're on their own, but in that case they're probably git-savvy enough to handle setting --git-ref also.

But I don't think it would be the command used most often. At least it wouldn't be in my workflow. I would be changing code, and looking at the test comparison, and then when I was happy with it I would commit, at which point I would be happy considering that the new reference. But I guess other people develop in different ways...

It seems like we have to impose some restrictions on this, as we can't reasonably handle all possible development practices. Having to have some people use a slightly different version of the command to accommodate their particular development process doesn't seem to onerous to me.

But again I reiterate my claim that the current stored reference method is not going to work, and that this method is better. The only issue seems to be whether we 'HEAD' or 'master' or 'origin/master' as the new "reference". We could just try one for a while and see how it works...

Yes, it certainly depends on your workflow. I most often use gwinc.test before pushing, to preview what the MR is going to report.

Anyway, this MR says it requires two approvals, so maybe @evan.hall or @kevin.kuns would like to weigh in on what the default gwinc.test reference should be.

Isn't the point of this to ensure that noise calculations do not change until we're really sure that they should? In that case I think the reference should always be what's being referred to here as origin/master, though upstream/master seems like another reasonable possibility for this name. I understand why you may want to compare with some other reference, but I'm missing why that would be the most common thing to compare against.

Let me clarify. When a merge request is submitted, the CI always compares the HEAD of MR source branch against origin/master. I think this is always the right thing to do, as we want to compare whatever changes are being proposed by the MR with the current state of the code.

What we're quibbling about in this thread is what gwinc.test should compare against by default if you run it e.g. in your personal checkout of the code on your laptop. In the proposed implementation it compares against 'HEAD', which resolves to the last commit on whatever branch you're currently working on. I believe @christopher.wipf is arguing that it should compare against the current upstream HEAD, which for most users will likely be 'upstream/master' but in some case may be under a different name. My concern is that we can't ensure that 'upstream/master' is the same thing for all users, so using 'upstream/master' could cause confusion or errors in some cases. 'HEAD', on the other hand, always has the same meaning. In either case, users can choose any arbitrary reference to compare against with:

$ python3 -m gwinc.test --git-ref HEAD
$ python3 -m gwinc.test --git-ref origin/master
$ python3 -m gwinc.test --get-ref upstream/trunk
...

I guess I propose that we stick with 'HEAD' for the time being, and revisit if it turns out that it's inconvenient for most people's workflow. But I'd like to hear what works best given other people's workflow.

One could try to deduce from git remote -v what the correct branch name is on each user's local file system, and throw an error if the correct branch is not present. Then there is the question of how the test is supposed to know if the local branch is up to date with the remote. So it seems like trying to compare against a fixed master could be error-prone.

Maybe it's easiest to just leave HEAD as the default comparison.

Let's examine what would be required in doing something other than 'HEAD':

You have to pick something, so what would it be? Well the obvious would be the head of whatever target branch you're intending your changes to target. But that could be anything really. But let's assume everyone always wants to target the canonical upstream/master. But both the names "origin" and "master" could have different meaning for different users. So let's try to deduce the name in the users current config that points to the "canonical" upstream/master:

First you have to figure out the remote name. We could do something like this:

$ upstream_remote=$(git remote -v | grep gwinc/pygwinc.git | grep push | awk '{print $1}')

We could then set the default reference to "${upstream_remote}/master". That would probably work in most cases. But as @evan.hall points it might not necessarily be up-to-date. And this also assumes everyone is trying to merge to the canonical upstream master, and not to some other fork of the project under a different name/url.

I'm just pointing out that anything other than HEAD requires a more subtle approach that will lead to issues in some not too uncommon cases.

And of course the MR CI pipeline is always checking against the uptodate origin/master, so we know that comparison will always be correct, whatever we choose the default reference to be.

I would just like to get some consensus from folks so we can push ahead and get the rest of the outstanding MR merged. The current method of storing a reference in the code I continue to maintain won't work in the long run.

I think this discussion is just about what the default behavior should be since you can specify any reference. Ignoring how it's implemented, it seems like you'd most often want to compare with upstream/master (i.e. gwinc/pygwinc.git). So since the above solution would work in most cases, why not do that?

If you compare with HEAD, shouldn't it trivially pass if you don't have any uncommitted changes? And if you do, you're just comparing with the last commit which seems likely to be wrong if you're developing something.

At the end of the day, I don't think it matters that much since you can specify what to compare with.

Approving because HEAD does have the advantage of a consistent and unique definition, and we don't seem to be approaching consensus on using anything else.

approved this merge request

merged

mentioned in commit 8fb7e709

simplify testing by removing unnecessary reference

Merge request reports

Activity