Skip all performance tests if not in a git repo.
ClosedPublic

Authored by DavidEichmann on Nov 21 2018, 4:39 AM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
DavidEichmann created this revision.Nov 21 2018, 4:39 AM
  • Correct assignment of hasMetricsFile.
DavidEichmann retitled this revision from Skip all perormance tests if not in a git repo. to Skip all performance tests if not in a git repo..Nov 21 2018, 5:21 AM
tdammers requested changes to this revision.Nov 26 2018, 5:23 AM
tdammers added a subscriber: tdammers.
tdammers added inline comments.
testsuite/driver/perf_notes.py
28

Can we be a bit more precise here? As written, this says "if anything goes wrong at all while running the git subprocess, assume we're not inside a git repo"; if the git subprocess exits without error, but doesn't produce any output, also assume we're not inside a git repo. I think this is too crude - at the very least, I would distinguish CalledProcessError from other exceptions, and make the "if subprocess output is empty" check more explicit than casting to bool.

testsuite/driver/runtests.py
366

The messaging is a bit awkward here, because our "are we in a git repo" tests don't distinguish between "this actually is not a git repo", "you don't have git installed", and other failures (such as "you don't have read permissions on the CWD" etc.).

374

consider reordering the logic here into something like:

if isGitRepo:
    reason = 'the previous...'
else:
    reason = 'this is not a git repo...'

Easier to follow than overwriting reason in place.

This revision now requires changes to proceed.Nov 26 2018, 5:23 AM
osa1 requested changes to this revision.Nov 26 2018, 6:32 AM
osa1 added a subscriber: osa1.

I already wrote this in the ticket 5 days ago, but I have a problem with this idea. It doesn't make sense to me that we're skipping an entire set of tests because we're not inside a git repo. After all the tests have nothing to do with git (it's not like we're testing something about git or the GHC git repo). It seems to me that this is implementing hack to avoid a problem that should be actually fixed somehwere else (namely, the problem of trying to add perf test results in git notes).

Why is it so hard to avoid adding git notes when not inside a git repo instead?

I'll request changes because of the idea (haven't even looked at the code yet).

  • Replace is_git_repo with can_git_status.

@osa1 We use git notes to get the baseline performance numbers and to save the new numbers. Here we only skip performance tests if we both cannot get a baseline (and hence can't tell if tests pass or fail) nor have a way to save the results.

testsuite/driver/perf_notes.py
28

I didn't manage to find a git command that would reliably say if the current directory is a git repo. You'd expect "git rev-parse --is-inside-work-tree" to print false if not in a work tree, but it actually exits with a non-zero exit code. Perhaps this is cleaner now: check that "git status" returns a 0 exit code, rename to can_git_status() and adjust warning messages/variable names to be a little more specific to what went wrong.

  • Fix logic to call Perf.append_perf_stat() only if canGitStatus.
  • Improve error message.
  • Use git status instead of rev-parse
osa1 resigned from this revision.Nov 28 2018, 5:58 AM
tdammers accepted this revision.Nov 28 2018, 9:59 AM

Looks good to me!

This revision is now accepted and ready to land.Nov 28 2018, 9:59 AM
This revision was automatically updated to reflect the committed changes.
ggreif added a subscriber: ggreif.Nov 30 2018, 11:24 AM

I suggest adding the -uno option, as it cuts off a few seconds form the command's runtime.