Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Testing] Organizing test utilities package(s) #609

Closed
2 of 9 tasks
bryanchriswhite opened this issue Mar 23, 2023 · 4 comments · Fixed by #685 or #696
Closed
2 of 9 tasks

[Testing] Organizing test utilities package(s) #609

bryanchriswhite opened this issue Mar 23, 2023 · 4 comments · Fixed by #685 or #696
Assignees
Labels
code health Nice to have code improvement testing Defining, adding, automating or modifying tests

Comments

@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Mar 23, 2023

Objective

Agree on a convention which we can document that establishes where to put common test code for reuse between packages.

Origin Document

image

Goals

  • Agree on a conventional place to organize test utilities packages (e.g. pocket/internal/testing)
  • Support de-duplication of test code across packages (i.e. test util pkgs are importable )
  • Ensure test packages are excluded from non-test builds (e.g. using build constraints: //go:build test)

Deliverable

Non-goals / Non-deliverables

  • Identifying additional duplication
  • Refactoring unrelated test code

General issue deliverables

  • Update the appropriate CHANGELOG(s)
  • Update any relevant local/global README(s)
  • Update relevant source code tree explanations
  • Add or update any relevant or supporting mermaid diagrams

Testing Methodology

  • Ensure test util package members are not present in symbol table of non-test builds:

    1. go build -o ./pokt-node ./app/pocket
    2. nm ./pokt-node | grep prepareDNSResolverMock should print nothing
      If prepareDNSResolverMock was included in the build, it will be listed in the symbol table which we can print with nm. Double-check the function names match, a typo could result in a false positive. Additionally, one can build a test binary (go test -c -o <output bin path> [-run <test func name>] <pkg>) on a test/pig which is known to import a test util function and ensure that it is present in the symbol table, as a control.
  • All tests: make test_all

  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md


Creator: @bryanchriswhite
Co-Owners: @deblasis

@bryanchriswhite bryanchriswhite added code health Nice to have code improvement testing Defining, adding, automating or modifying tests labels Mar 23, 2023
@bryanchriswhite bryanchriswhite moved this to Backlog in V1 Dashboard Mar 23, 2023
@bryanchriswhite bryanchriswhite self-assigned this Mar 23, 2023
@bryanchriswhite bryanchriswhite linked a pull request Mar 23, 2023 that will close this issue
17 tasks
@Olshansk
Copy link
Member

@bryanchriswhite Just wanted to follow up with some additional context

  1. Fully support this idea. It was always the plan but just never a priority.
  2. Example: We copy-paste our functions similar to basePersistenceMock for mocking, which should 💯 % be shared in a common location.
  3. We have some shared prod related utilities in shared/utils/* so shared/testing/* could follow the same pattern (for now).
  4. In [Core] Follow industry standards for Golang project folder structure #379, @Gustavobelfort started implementing a standard go file structure (i.e. using internal where appropriate), but we never finished it. The initial approach can be found in [Core] Restructure repo to follow industry golang standards #412.
  5. I want to know what we're documenting!

Screenshot 2023-03-23 at 12 35 12 PM

@bryanchriswhite
Copy link
Contributor Author

bryanchriswhite commented Mar 24, 2023

f@Olshansk Haha - yea, I must have gotten distracted. Thanks for calling this out! 🙌 Sentence finished. ✔️ 😉

Thanks for the links! 🙌 I read through to golang-standards/project-layout which (great resource - thanks @Gustavobelfort) describes /internal like so:

Private application and library code. This is the code you don't want others importing in their applications or libraries. Note that this layout pattern is enforced by the Go compiler itself. See the Go 1.4 release notes for more details. Note that you are not limited to the top level internal directory. You can have more than one internal directory at any level of your project tree.

I would argue that given the convention, would better communicate our intention to put cross-package test code in an internal pkg. It would seem that it doesn't matter where that internal package lives; i.e., it gets the same treatment from the compiler and should be interpreted the same by humans. If you feel strongly about being downstream from /shared, which I can understand, I think /shared/internal or /shared/internal/testing could be a nice compromise.

EDIT: After mulling it over a bit more, I realize that /shared/internal would make the conventional package name internal which I would argue isn't great. Additionally, /shared/internal/testing would make the conventional package name testing, which collides with the stdlib testing package. My updated proposal is /shared/internal/testutils.

UPDATE:

There's a detail that the project-layout /internal README seems to have omitted but is the third sentence of the release doc section it refrences:

To create such a package, place it in a directory named internal or in a subdirectory of a directory named internal. When the go command sees an import of a package with internal in its path, it verifies that the package doing the import is within the tree rooted at the parent of the internal directory. For example, a package .../a/b/c/internal/d/e/f can be imported only by code in the directory tree rooted at .../a/b/c. It cannot be imported by code in .../a/b/g or in any other repository.

(e.g.: in pocket/p2p/module_raintree_test.go)
image

@Olshansk it would seem that to achieve the goal, we have no choice but to create a pocket/internal package. 🤔 My updated proposal would be to go with a single top-level /internal pkg for now and if we find that we want/need to split the package up and/or re-scope parts to module specific internal directories we can always do that later (and I assume for less than or equal effort than to go the other direction).

@bryanchriswhite
Copy link
Contributor Author

It also looks like build tags/constraints have changed since the last time I looked:

Go versions 1.16 and earlier used a different syntax for build constraints, with a "// +build" prefix. The gofmt command will add an equivalent //go:build constraint when encountering the older syntax.

...

A build constraint, also known as a build tag, is a condition under which a file should be included in the package. Build constraints are given by a line comment that begins

//go:build
Build constraints may also be part of a file's name (for example, source_windows.go will only be included if the target > operating system is windows).

See 'go help buildconstraint' (https://golang.org/cmd/go/#hdr-Build_constraints) for details.

@jessicadaugherty jessicadaugherty moved this from Backlog to Up Next in V1 Dashboard Mar 27, 2023
@bryanchriswhite bryanchriswhite linked a pull request Apr 17, 2023 that will close this issue
19 tasks
@bryanchriswhite bryanchriswhite moved this from Up Next to In Review in V1 Dashboard Apr 17, 2023
bryanchriswhite added a commit that referenced this issue Apr 19, 2023
…685)

## Description

Factors out some common `mockdns` usage, including a "noop" logger to
things down (`mockdns` is noisy IMO).

## Issue

Fixes #609 

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Create `/internal` directory
- Add `/interal/testutil` pkg
- Refactor existing `mockdns` usage

## Testing

- [x] Ensure test util package members are not present in symbol table
of non-test builds:
  1. `go build -o ./pokt-node ./app/pocket`
  2. `nm ./pokt-node | grep prepareDNSResolverMock` should print nothing
If `prepareDNSResolverMock` was included in the build, it will be listed
in the symbol table which we can print with `nm`. **Double-check the
function names match, a typo could result in a false positive**.
Additionally, one can build a test binary (`go test -c -o <output bin
path> [-run <test func name>] <pkg>`) on a test/pig which is known to
import a test util function and ensure that it is present in the symbol
table, as a control.


![image](https://user-images.githubusercontent.com/600733/232450913-0e5c35ad-7734-404c-a563-d10a7b6f00b9.png)


- [ ] `make develop_test`; if any code changes were made
- [ ] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [ ] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

## Required Checklist

- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
@bryanchriswhite bryanchriswhite moved this from In Review to Done in V1 Dashboard Apr 19, 2023
@Olshansk
Copy link
Member

@bryanchriswhite Should this ticket be in the Done state?

bryanchriswhite added a commit that referenced this issue Apr 21, 2023
…de (#696)

## Description

Factors out some common mockdns usage, including a "noop" logger to
things down (mockdns is noisy IMO).

(Duplicate of #685 🙄, I forgot to change the base branch
before merging. As a result, the changes were squashed-merged into a
different branch than main. Re-opening here, apologies for the extra
noise. 🙏🙏)

## Issue

Fixes #609

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Create `/internal` directory
- Add `/interal/testutil` pkg
- Refactor existing `mockdns` usage

## Testing

- [x] Ensure test util package members are not present in symbol table
of non-test builds:
  1. `go build -o ./pokt-node ./app/pocket`
  2. `nm ./pokt-node | grep prepareDNSResolverMock` should print nothing
If `prepareDNSResolverMock` was included in the build, it will be listed
in the symbol table which we can print with `nm`. **Double-check the
function names match, a typo could result in a false positive**.
Additionally, one can build a test binary (`go test -c -o <output bin
path> [-run <test func name>] <pkg>`) on a test pkg which is known to
import a test util function and ensure that it is present in the symbol
table, as a control.


![image](https://user-images.githubusercontent.com/600733/232450913-0e5c35ad-7734-404c-a563-d10a7b6f00b9.png)


- [ ] `make develop_test`; if any code changes were made
- [ ] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [ ] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

## Required Checklist

- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment