search

celestiaorg / smt

A Go library that implements a Sparse Merkle tree for a key-value map.
https://godoc.org/github.com/celestiaorg/smt
MIT License
138 stars 53 forks source link

Remove or move bench test #45

Closed musalbas closed 3 years ago

musalbas commented 3 years ago

Does it make sense to make a benchmark test as part of the main test suite? It's not testing any software assurances, and just making running the tests slower.

IMO this should be removed, or moved to a different package.

https://github.com/celestiaorg/smt/pull/42

liamsi commented 3 years ago

Tests ran in less than a minute which is still fast enough IMO (and no real difference between past CI runs). But if you want, you can just skip the test. I do think it makes sense to have benchmarks as part of the main test suite. The benchmarks can also be used to determine that future changes do not slow down things.

musalbas commented 3 years ago

Right, but shouldn't it at least be in sub-directory/package called bench/ or something? Otherwise it's cluttering the main folder (and default tests) with non-critical code/tests.

liamsi commented 3 years ago

Sure, I'd just say benchmarks as long as they do not run annoyingly long, could just stay part of the default test-suite. They are clearly marked as benchmarks via their name so they do not create confusion or anything. But yeah, in general, it might to be a good practice to have some more public API test, e.g. by having a dedicated suffix "_test" package. IMO, it's not common to have another special package for tests (which benchmarks are a special case of) unless you are doing integration tests spanning multiple packages. But if you feel really strongly about this, it's up to you. Just some examples of the go standard library where small benchmarks are part of the same package:

liamsi commented 3 years ago

Also, benchmarks are only:

executed by the "go test" command when its -bench flag is provided.

https://pkg.go.dev/testing#hdr-Benchmarks