In tests, replace go mod download with a mock executable

[fedepaol]

What is the problem I am trying to address?

As described in #1359 , we can test the interaction with the go executable bt replacing it with a local mock backed by a test function, as described in https://www.youtube.com/watch?v=8hQG7QlcLBk
By doing this, we are not testing the interaction with the real go executable but since we are not hitting github any more the execution of those tests is faster.

There are still a couple of tests that need the real executable.

Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.

Fixes #1359
Edit from @arschles: Fixes #1368

[dylan-bourque]

While it technically works, I would argue against shadowing the new package-level cmd variable like this. Either rename the package variable or use a different local variable.

[marwan-at-work]

In most cases, we certainly should not mock go mod download this is for a few reasons:

1. Athens has a hard-dependency on the Go command. In other words, if the Go command does not exist, Athens will not work.

2. If we mock the Go command, then we're not gonna be able to catch subtle differences in evolving Go versions. Let's say Go released v1.15 and they accidentally broke go mod download, our tests will at least catch those.

Is there a use-case I'm missing here? Thanks ✌️

[fedepaol]

In most cases, we certainly should not mock go mod download this is for a few reasons:

1. Athens has a hard-dependency on the Go command. In other words, if the Go command does not exist, Athens will not work.

2. If we mock the Go command, then we're not gonna be able to catch subtle differences in evolving Go versions. Let's say Go released v1.15 and they accidentally broke `go mod download`, our tests will at least catch those.

Is there a use-case I'm missing here? Thanks v

@marwan-at-work that was my understanding as well, but the investigation issue was open and it looked something fun to implement :-)
I totally agree if we want to keep this out for the reasons you just highlighted.

[dylan-bourque]

To me, the value-add is that this change allows the unit tests to be run in complete isolation (i.e. no network, no fetching a repo from GH, no waiting on that repo to actually be copied across the wire, etc.). Also, what the test is currently asserting is that go mod download writes a specific set of files to "disk". IMO, that's not a test of the Athens logic, that's testing that go mod download can download a module. What the unit test should be doing is verifying that it's invoking the go CLI tool correctly.

I feel that verifying the end-to-end Athens->go mod download-> correct content on disk is an integration test (i.e. part of higher-level QA) rather than a code-level unit test, but you know what they say about opinions. :)

[marwan-at-work]

From slack:

Athens doesn't really have much logic besides "download a module and store it".

Any logic that doesn't use go mod download (such as the Filter File and the Download File), they already run their unit tests without the Go binary (AFAIK).

For example, our Storage tests don't use the Go binary because the Storage layer doesn't care where the "bytes" come from, we just care to unit test the storage's CRUD operations.

As for the network and isolation, if someone doesn't have a "network" or the right "go" binary, then the tests should fail, because Athens effectively won't work in that isolated environment.

it might be worth identifying exactly which unit tests should not use the go binary?

[fedepaol]

So, there are 5 tests covered in go_get_fetcher_test, and two of them are already using the real binary (one is the TestGoGetFetcherError which needed to have a non existing binary).

What we test there is the interaction with the go binary (parsing the resulting json, and behaving accordingly, like checking the various files are where the json say they are). I don't have a strong opinion on this. On one hand we are testing in isolation (and the tests run waaaay faster), on the other hand we may skip breaking changes in how go behaves and testing against real go is not that expensive. So, I don't know 😟 . @arschles any opinions on this?

[arschles]

I think this touches on a discussion of what the difference between unit, integration and end-to-end tests is. I agree that for now, our tests should holistically be covering Athens' interaction with the real go CLI.

Without going too out of scope of this PR, I personally think we should increase test coverage from our end-to-end tests (test_e2e.sh) before we do this PR.

As always, I certainly don't want my opinion to become the decision, and I'm 💯% open to having a vote or more discussion on this!

Background: #1359 came from a discussion in a previous office hours

[arschles]

Ref #1368 because I think this would close that too

[marwan-at-work]

my vote is to keep things the same since go mod download is still not finalized in many ways (such as error codes and consistent json output)

[dylan-bourque]

I'm fine with whatever path, although I still believe it's a good idea to have unit tests without external dependencies (even for code that is using exec.Cmd to run a sub-process)

[arschles]

@fedepaol what do you think about expanding the end-to-end tests a bit instead of adding the mock executable to unit tests?

[fedepaol]

@arschles not sure I understood.
I agree with the fact that the interaction with go as a process belongs to e2e, while unit tests should be fast & local. On the other hand, testing if the interaction between go & athens break is more important.

Ideally, we could replace the (basic) e2e script with something more articulate (which makes sense), and once we have wide coverage of the interaction with the go process in e2e we can merge this and have the local test be faster.

(Or you are just suggesting to expand the e2e tests a bit, not related to this pr)

[arschles]

(Or you are just suggesting to expand the e2e tests a bit, not related to this pr)

@fedepaol yup, that's exactly what I'm suggesting. Once we have expanded e2e tests, we could then introduce this PR with the mock executable. What do you think?

[fedepaol]

@arschles sounds like a plan.
Will mean replacing the bash script we currently have with something like a test suite doing syscalls (or something on that line).

[arschles]

@fedepaol makes sense, thanks for creating that issue 😄

[arschles]

I'm going to label this on hold until #1474 is done

[arschles]

@fedepaol now that your #1514 is merged (thank you again!), do you feel like continuing with this PR?

[marwan-at-work]

Just my 2 cents, I still don't feel confident in mocking go mod download and go list. At the very least, we should do the following two things first:

1. Cover all the cases where we actually test list/moddownload in the e2e tests, things like transitive dependencies, pseudo semver, and SIV.

2. Implement Athens in offline mode. Reason being, is that currently Athens is totes useless without a valid Go binary, so if someone tries to run "go test ./..." and they don't have a "go" binary, then Athens succeeding is actually a false positive...because running the tests will give you a false sense of "Athens is working", but the moment they run Athens and use it, things will fail. However, once we have offline mode, we'll have a version of Athens that doesn't really need the Go binary and therefore it makes sense...but even then, offline mode is the rare case, and I still appreciate having the tests ensuring Athen's basic usage being covered.

[fedepaol]

Either continuing (or even trying to implement 1 in e2e) or closing this is fine to me, as long as we reach consensus on bringing this forward.

Next time I'll wait a bit longer and ask around first, so I am sure the issue I am implementing is something we really want, attempting to reduce the waste of time and helping on some other issue.

[arschles]

It sounds like we have a long way to go before we can continue with this, because we need to do (1) and offline mode. Let's keep this open and keep the on hold tag. We can come back to it whenever those two things are done.

[fedepaol]

Closing, we might repoen if needed