Fall back to current time if git metadata is not available

[marcinwyszynski]

This change makes coverage reporting work from a Docker container built in CI with no git metadata (as this is not something you want persisted in your Docker image). I believe that current timestamp is a decent fallback value, since you'd usually run coverage reporter from a CI provider which is triggered by a commit. An alternative I think would be to allow passing commit timestamp as an environment variable.

[dblandin]

Hey @marcinwyszynski,

Our new test reporter supports a number of environment variables for the commited at timestamp:

GIT_COMMITTED_AT
GIT_COMMITED_AT
CI_COMMITTED_AT
CI_COMMITED_AT

https://github.com/codeclimate/test-reporter/blob/960f8b2d46e87dc8a37b97ab44bdad37903ce670/env/git.go#L163

You can find the docs for the new test reporter here: https://docs.codeclimate.com/docs/test-reporter

Alternatively, you could mount the .git directory into your docker container while running your test suite:

docker run --volume $(pwd)/.git:/path/to/app/.git your/image bundle exec rspec

I would recommend checking out the new test reporter. Hopefully one of these solutions works for you. Let me know!

[marcinwyszynski]

Thanks @dblandin, either of your suggestions will work. Still, this looks like a decent fallback?

[pbrisbin]

I'm not necessarily against having this defaulting client-side, but this value is not a required attribute in the payload and we will default to "now" when we receive it server-side.

[marcinwyszynski]

@pbrisbin That's not what I've seen. First, CodeClimate:: TestReporter::PayloadValidator#validate will raise InvalidPayload. Second, I was manually sending the payload with a null value and while I got 200 OK from your server, the coverage was never reported.

[marcinwyszynski]

In similar vein, the new test reporter would still fail in my scenario, except if I manually added a dummy timestamp variable (since CircleCI does not expose it). If that value is optional and not required on your end, perhaps let's not make it required in either reporter?

[pbrisbin]

I'm sorry @marcinwyszynski, I must've been incorrect (or out of date). I'll leave the decision to accept this patch (and push it through to a release) to another teammate. Given that the old reporters are very near deprecated, if we do want this defaulting, we might want to do it only in the new test-reporter codebase.

[dblandin]

I'm hesitant to accept this patch if solutions exist that will pass along the correct value. The current time might be close but does not accurately reflect when the commit was committed.

The committed at timestamp is important today when we're deciding whether to render test coverage annotations on github.com via the browser extension. We likely use this timestamp on codeclimate.com as well. I think it's probably better to encourage reporting with the correct data.

I can think of a few cases that might lead to non-trivial drifts between the real committed at time and Time.now:

What if a CI run is rebuilt?
What if a CI run is delayed (ex: due to queuing)
What if a webhook from GitHub is delayed?

I'd personally rather error with a meaningful message, leading to an approach that will send the right data.

For Circle CI specifically, until they have an environment variable to thread through, volume-mounting the .git directory should work.

@marcinwyszynski What do you think?

[marcinwyszynski]

Mounting .git sounds like the tail wagging the dog, but I see your point.

[dblandin]

Mounting .git sounds like the tail wagging the dog, but I see your point.

Yep, definitely not ideal but I think it's better than allowing inaccurate data through. I'll reach out to Circle CI to see if they might add a COMMITTED_AT env var.

Thanks for the discussion! 👍

[marcinwyszynski]

@dblandin Unfortunately I'll have to reopen it because volume mounting does not work in Circle CI.