golang package and test best practice about sabre HOT 8 OPEN

While I agree the issue is real and there should be a way to solve this (and there is; see below), I do not agree that using same package is considered best practice (I do make exceptions to this rule, for example reflect_test.go, but my preference is separate package). In fact, many other Gophers recommend this too:

1. https://medium.com/@matryer/5-simple-tips-and-tricks-for-writing-unit-tests-in-golang-619653f90742
2. https://medium.com/@benbjohnson/structuring-tests-in-go-46ddee7a25c

Also, pkg is a common practice for large application-like projects with lot of internal and publicly reusable packages, where it provides a way to explicitly segregate internal and reusable packages. But for a project that is meant to be used as a library where every package is public by design, this practice is not usually followed since it doesn't really provide any added benefit in my experience. In Sabre for example, if we do create a pkg, what would be kept outside of it?.

Coming back to the issue for forked versions: I would be okay to go with approach proposed by @lthibault if it solves the issue, but it doesn't completely since the import path issue still remains for integration_test.go file. If there are edge cases like this, may be it isn't the best possible solution.

There are 2 cases where users would want to fork:

1. To make some change and send a PR upstream: In this case, forked version is temporary and hence it would be good idea to clone the original repo and work on it directly. Changes can be pushed to the forked repository by adding it as another remote. For example, in @leifj case, the approach should be: Clone Sabre repo, run git remote add forked github.com/leifj/sabre, make changes, do git push master forked/master (Preferably --set-upstream forked can also be done to keep the local in sync with forked remote). This is usually the recommended approach for Go projects as explained here and here and here etc. This approach works well since it allows to make changes safely to forked version and also allows following the testing practice. (This is the approach I follow usually as well).
2. Origin is not maintained or the developer has some plans to take the forked version in different direction than origin: In this case, it is required to change all the import paths to the forked git repo URL anyway (Since, users of the forked version would want to use github.com/leifj/sabre as the import path)..

The issue of working with a 3rd project by importing the forked version will still remain with the 2 remote approach mentioned above. But I think it is a bad idea to work on a 3rd project with the assumption that the changes will be merged upstream: PR discussion etc are happening precisely to discuss whether they should be merged to upstream or not. May be a different design is expected by maintainers or may be PR gets rejected. What happens to the project depending on the fork then?. Also, it is quite possible that the changes done on forked version get heavily influenced by that 3rd project as well..

But, In worst case if this is still needed (may be maintainers are taking too long to respond and you don't want to get blocked with your project), you can make use of replace directive in Go mod.

Let me know if this works. 🙂

from sabre.

Regarding tests: this suggestion was previously discussed, and I recall @spy16 was opposed to moving tests into the same package as code. IIRC, his position was that placing tests in a separate package forces developers to test the behavior of the high-level API rather than testing implementation details. The rationale is that this decoupling of tests from implementation is more robust to changes in the codebase. Personally, I'm not opposed to moving tests into the sabre package, but I'd like to get Shivaprasad's input on the matter.

Regarding the standard go layout: I'm not opposed to this as I make heavy use of /pkg, myself. That said, I'm not sure I understand why/how the current package layout forces you to modify every file, nor how moving public code to /pkg addresses the problem. Could you elaborate?

To reiterate: I'm mostly onboard with the proposed changes, but just want to make sure I understand both the problem and the solution. The package layout has been an annoyance in the past, so this may well be a good time to fix it.

As usual, thanks for your contributions!

from sabre.

The basic problem is that if I want to do a PR I have to modify a bunch of files to replace the temporary package name instead of just keeping my own copy of go.mod

from sabre.

I'm not sure I see where temporary packages or changes to go.mod are needed. In case it helps identify a misunderstanding, my mental model for your workflow is this:

1. You've forked this repository and cloned it
2. You make changes to your fork. go test, go build, go run, etc work as expected.
3. Once satisfied, you commit & push to your fork's repo on github.
4. You open a PR

Am I overlooking something? I realize it can be tedious to explain/reproduce these issues, but this kind of feedback is really precious -- it helps us ensure we support as many workflows as possible.

Overall, this is a sensible proposal and it has my support, pending @spy16's input regarding the sabre_test package.

from sabre.

Yeah all the tests depend on the spy16 version so I'm not actually testing my code with that approach afiu

from sabre.

I thought this problem was solved with go mod since go tools are now looking at the go.mod file to check what module the files are in. So when they see gtihub.com/spy16/sabre as import path in test files, since they are already inside that module, they should be using the local copy of the forked version. Is this not happening?

from sabre.

I don't see how. Remember that I need to be able to test and use leifj/sabre as a dependency of other projects while we talk about PRs etc. So I clone my fork of sabre and make changes. I type make - go build identifies spy16/sabre as a dependency of leifj/sabre via the tests.

However this may all be a moot point if it turns out 'case' is just a macro.

from sabre.

Remember that I need to be able to test and use leifj/sabre as a dependency of other projects while we talk about PRs etc

Aha! This was the missing piece! 😃

However this may all be a moot point if it turns out 'case' is just a macro.

In this particular case, yes, but I still think this is a problem worth fixing.

@spy16 I can relate to your reasons for wanting the tests to live in a separate package. What do you think of doing the following?

1. Move tests to sabre package.
2. Add integration_tests.go, which specify tests that live in the sabre_test package.
3. Enforce a policy of writing tests that interact only with exported types/methods (exceptions can be considered if justified).

I realize it's a bit less clean than enforcing this through separate packages, but it may be a good compromise for the sake of users in @leifj 's position.

from sabre.