tests: add command to compile all test files #7364

robert-zaremba · 2020-09-22T14:58:57Z

test-build-check will compile and use go cache mechanism to
check if ALL files compiles without running any test.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

test-build-check will compile and use go cache mechanism to check if ALL files compiles without running any test. ref: 7362

Makefile

alessio

This seems a tad unnecesary. Go's cache is enabled by default, and so it must be in order to use go modules. Plus make test-all already runs all tests and avoid re-execution by leveraging Go's internal cache.

robert-zaremba · 2020-09-22T16:00:24Z

make test-all

doesn't cache all tests, (eg: ok github.com/cosmos/cosmos-sdk/server/grpc 17.180s ),

it takes lot of time, subsequent run takes:

  make test-all  172.39s user 10.43s system 127% cpu 2:23.07 total

Running make test-all makes me going for a break.

On the other hand, subsequent run of make test-build-check:

make test-build-check  53.37s user 6.96s system 326% cpu 18.455 total

And we can optimize it way more if we will use go test -c in each package.

robert-zaremba · 2020-09-22T16:02:14Z

not sure how much time it would take to go and run go test -c in all subdirectories.

alessio · 2020-09-22T16:17:16Z

doesn't cache all tests

Untrue. It does.

subsequent run of make test-build-check

...bring no advantages and the Makefile rule execution is faster just because it does not run all tests.

robert-zaremba · 2020-09-22T18:22:20Z

...bring no advantages and the Makefile rule execution is faster just because it does not run all tests.

As it's noted in the make task, that's the whole goal to not execute any test and only do a compilation.

alessio · 2020-09-22T18:40:07Z

As it's noted in the make task, that's the whole goal to not execute any test and only do a compilation.

Then test-build-check is a misnomer. All test* targets in our Makefile run tests. This wouldn't. Most importantly, what is the benefit of it? What other reason you want to compile your test packages for if not to actually run tests?

robert-zaremba · 2020-09-22T19:00:30Z

What other reason you want to compile your test packages for if not to actually run tests?

I already mentioned it -> save some time. If I'm doing some small refactoring, especially in a core package, I don't want to wait 6-10min each time to re-execute all tests. I firstly will like to check that everything compile and then after fixing compilation issues, I would run tests.

alessio · 2020-09-22T19:23:42Z

I don't want to wait 6-10min each time to re-execute all tests

You have to. Just compiling your tests is not enough. Your editor (with the right settings, e.g. build tags) can handle everything for you.

I already mentioned it -> save some time

Again, your editor does it for you. Adding a tests-compilation target in the Makefile that fundamentally does nothing is pointless.

alessio · 2020-09-22T19:26:24Z

I already mentioned it -> save some time

go test -mod=readonly -run=nope ./... does not save time at all.
When you run your tests with the various tags (and we need to do so for various reasons), you'll inevitably rebuild them.

robert-zaremba · 2020-09-22T19:51:09Z

You have to. Just compiling your tests is not enough.

Please re-read my comment above.

go test -mod=readonly -run=nope ./... does not save time at all.

6min vs 18sec is saving a time. I did it few times today and it helped me and saved my time a lot (BTW: it has noting to do with running a particular test).

Anyway, if it's only for me then I will add it to private scripts and update .gitignore so we can have private scripts.

alessio · 2020-09-22T22:16:38Z

6min vs 18sec is saving a time.

It is not, especially if the compiler has to recompile regardless of whether you had compiled before. Again, try dig into your go tests cache and you will see it yourself - different +build tags mean different objects, thus if you build your test objects without a specific build tag, and you the run exactly those tests with some build tags, the compiler will rebuild the test objects anyway.

Anyway, if it's only for me then I will add it to private scripts and update .gitignore so we can have private scripts.

That sounds like an eminently wise choice.

alessio · 2020-10-20T20:24:19Z

Bump - can we close this for now please?

robert-zaremba · 2020-10-20T20:44:20Z

I'm using test-build-check through a private Makefile. So, if nobody needs it then let's close.

alessio · 2020-10-20T20:48:54Z

I drop here a replacement of the proposed patch, which would cause tests to be effectively cached and not rebuilt (provided that one uses exactly the same tags in its build configuration):

diff --git a/Makefile b/Makefile
index bd667b1c0..0ee675b1d 100644
--- a/Makefile
+++ b/Makefile
@@ -218,6 +218,7 @@ test-all: test-unit test-ledger-mock test-race test-cover
 
 TEST_PACKAGES=./...
 TEST_TARGETS := test-unit test-unit-amino test-unit-proto test-ledger-mock test-race test-ledger test-race
+CHECK_TEST_TARGETS := check-test-unit check-test-unit-amino check-test-unit-proto check-test-ledger-mock check-test-race check-test-ledger check-test-race
 
 # Test runs-specific rules. To add a new test target, just add
 # a new rule, customise ARGS or TEST_PACKAGES ad libitum, and
@@ -230,13 +231,22 @@ test-ledger-mock: ARGS=-tags='ledger test_ledger_mock norace'
 test-race: ARGS=-race -tags='cgo ledger test_ledger_mock'
 test-race: TEST_PACKAGES=$(PACKAGES_NOSIMULATION)
 
+check-test-unit: test-unit
+check-test-unit-amino: test-unit-amino
+check-test-ledger: test-ledger
+check-test-ledger-mock: test-ledger-mock
+check-test-race: test-race
+
 $(TEST_TARGETS): run-tests
 
+$(CHECK_TEST_TARGETS): run-tests
+$(CHECK_TEST_TARGETS): EXTRA_ARGS=-run=none
+
 run-tests:
 ifneq (,$(shell which tparse 2>/dev/null))
-       go test -mod=readonly -json $(ARGS) $(TEST_PACKAGES) | tparse
+       go test -mod=readonly -json $(ARGS) $(EXTRA_ARGS) $(TEST_PACKAGES) | tparse
 else
-       go test -mod=readonly $(ARGS) $(TEST_PACKAGES)
+       go test -mod=readonly $(ARGS) $(EXTRA_ARGS) $(TEST_PACKAGES)
 endif
 
 .PHONY: run-tests test test-all $(TEST_TARGETS)

robert-zaremba · 2020-10-21T08:15:05Z

looks good. Do you want to commit it here?

robert-zaremba · 2020-10-21T08:15:48Z

BTW: we only really need to build all tests, we don't need to split them in multiple categories.

alessio · 2020-10-21T08:37:10Z

we only really need to build all tests

If you build all tests with the same flags, cache won't work when you run make test-$WHATEVER, e.g. if you run go build ./... first, then make test-unit tests won't be cached. See my comment above for an explanation.

For what concerns me, this could be closed and my patch should be adopted. @robert-zaremba feel free to close this yourself. Thanks!

robert-zaremba · 2020-10-21T08:47:19Z

OK. @alessio - so you prefer to close this and make a new PR with your patch? That works for me. Do you want me to do it?

alessio · 2020-10-21T13:08:58Z

I don't believe it brings much benefit, I will hardly use it. But if you want and need it, feel free to either amend this PR or open a new one and I'll approve it 👍

robert-zaremba · 2020-10-21T19:09:59Z

@alessio , I've updated the PR. Added only check-test-unit and check-test-unit-amino. I the entry I created before.

Makefile

aaronc

ACK

tac0turtle · 2020-10-26T13:35:53Z

Makefile

+CHECK_TEST_TARGETS := test-unit test-unit-amino
+check-test-unit: test-unit
+check-test-unit-amino: test-unit-amino
+$(CHECK_TEST_TARGETS): EXTRA_ARGS=-run=none


is this suppose to be inserted on make test? I cant find any updates in the contributing on how to run tests?

cosmos-sdk on  marko/remove_toTM2 [!?] via 🐹 v1.15.3 on ☁️ markobaricevic3778@gmail.com ❯ make test go test -mod=readonly -tags='cgo ledger test_ledger_mock norace' -run=none ./...

cc @alessio @robert-zaremba

It introduces new jobs, but it doesn't change a way how you run tests. The new jobs are to compile and not runing any test.

but the old command to run tests doesn't run tests, so it does change how you run tests.

@marbar3778 fix is here - mind opening a PR?

I will do it.

alessio · 2020-10-26T14:52:52Z

Makefile

 $(TEST_TARGETS): run-tests

+# check-* compiles and collects tests without running them
+# note: go test -c doesn't support multiple packages yet (https://github.com/golang/go/issues/15513)
+CHECK_TEST_TARGETS := test-unit test-unit-amino


That's the offendling line @marbar3778, this should be:

CHECK_TEST_TARGETS := check-test-unit check-test-unit-amino

Ah, right. Sorry didn't notice. Wrong copy-paste 🤕

tests: add command to compile all test files

97a4297

test-build-check will compile and use go cache mechanism to check if ALL files compiles without running any test. ref: 7362

robert-zaremba added T: Tests T: Dev UX UX for SDK developers (i.e. how to call our code) labels Sep 22, 2020

robert-zaremba requested review from aaronc, alessio, clevinson, amaury1093, blushi, anilcse and atheeshp September 22, 2020 14:58

robert-zaremba requested a review from alexanderbez as a code owner September 22, 2020 14:58

alessio reviewed Sep 22, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

alessio reviewed Sep 22, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

alessio suggested changes Sep 22, 2020

View reviewed changes

update comment

389b91f

makefile: remove update-swagger-docs

573f3d7

robert-zaremba mentioned this pull request Sep 25, 2020

Improve testing DevX #7362

Closed

11 tasks

This was referenced Oct 14, 2020

Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. #7526

Merged

Adding a rule for private files in gitignore #7535

Merged

robert-zaremba added 2 commits October 21, 2020 21:04

Merge branch 'master' into robert/test-build

c9acbda

Makefile: redoing the test check command

000da5b

robert-zaremba requested review from alessio and tac0turtle October 21, 2020 19:10

alessio reviewed Oct 21, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Update Makefile

555d6b3

alessio reviewed Oct 21, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Update Makefile

6699181

alessio approved these changes Oct 21, 2020

View reviewed changes

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 21, 2020

aaronc approved these changes Oct 21, 2020

View reviewed changes

mergify bot merged commit 55d7d0c into master Oct 21, 2020

mergify bot deleted the robert/test-build branch October 21, 2020 20:51

tac0turtle reviewed Oct 26, 2020

View reviewed changes

alessio reviewed Oct 26, 2020

View reviewed changes

alessio mentioned this pull request Oct 26, 2020

fix check-test-* targets #7675

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: add command to compile all test files #7364

tests: add command to compile all test files #7364

robert-zaremba commented Sep 22, 2020

alessio left a comment

robert-zaremba commented Sep 22, 2020

robert-zaremba commented Sep 22, 2020

alessio commented Sep 22, 2020

robert-zaremba commented Sep 22, 2020

alessio commented Sep 22, 2020

robert-zaremba commented Sep 22, 2020

alessio commented Sep 22, 2020

alessio commented Sep 22, 2020

robert-zaremba commented Sep 22, 2020

alessio commented Sep 22, 2020 •

edited

Loading

alessio commented Oct 20, 2020

robert-zaremba commented Oct 20, 2020

alessio commented Oct 20, 2020

robert-zaremba commented Oct 21, 2020

robert-zaremba commented Oct 21, 2020

alessio commented Oct 21, 2020

robert-zaremba commented Oct 21, 2020

alessio commented Oct 21, 2020

robert-zaremba commented Oct 21, 2020

aaronc left a comment

tac0turtle Oct 26, 2020 •

edited

Loading

robert-zaremba Oct 26, 2020

tac0turtle Oct 26, 2020 •

edited

Loading

alessio Oct 26, 2020

robert-zaremba Oct 26, 2020

robert-zaremba Oct 26, 2020

alessio Oct 26, 2020

robert-zaremba Oct 26, 2020

tests: add command to compile all test files #7364

tests: add command to compile all test files #7364

Conversation

robert-zaremba commented Sep 22, 2020

alessio left a comment

Choose a reason for hiding this comment

robert-zaremba commented Sep 22, 2020

robert-zaremba commented Sep 22, 2020

alessio commented Sep 22, 2020

robert-zaremba commented Sep 22, 2020

alessio commented Sep 22, 2020

robert-zaremba commented Sep 22, 2020

alessio commented Sep 22, 2020

alessio commented Sep 22, 2020

robert-zaremba commented Sep 22, 2020

alessio commented Sep 22, 2020 • edited Loading

alessio commented Oct 20, 2020

robert-zaremba commented Oct 20, 2020

alessio commented Oct 20, 2020

robert-zaremba commented Oct 21, 2020

robert-zaremba commented Oct 21, 2020

alessio commented Oct 21, 2020

robert-zaremba commented Oct 21, 2020

alessio commented Oct 21, 2020

robert-zaremba commented Oct 21, 2020

aaronc left a comment

Choose a reason for hiding this comment

tac0turtle Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

robert-zaremba Oct 26, 2020

Choose a reason for hiding this comment

tac0turtle Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

alessio Oct 26, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 26, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 26, 2020

Choose a reason for hiding this comment

alessio Oct 26, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 26, 2020

Choose a reason for hiding this comment

alessio commented Sep 22, 2020 •

edited

Loading

tac0turtle Oct 26, 2020 •

edited

Loading

tac0turtle Oct 26, 2020 •

edited

Loading