Run integration tests separately from unit tests #18304
Conversation
Jenkins BuildsClick to see older builds (29)
|
ilmotta
force-pushed
the
split-integration-tests-from-unit-tests
branch
3 times, most recently
from
996926e to
1b935a8
Compare
ilmotta
requested review from
ajayesivan,
J-Son89,
siddarthkay,
cammellos and
vkjr
ilmotta
changed the title
Makefile
Outdated
| yarn install | ||
| yarn shadow-cljs compile mocks && \ | ||
| yarn shadow-cljs compile test && \ | ||
| node --require ./test-resources/override.js target/test/test.js |
There was a problem hiding this comment.
Code duplication with unit-test and integration-test.
I would move to a shared task or script.
There was a problem hiding this comment.
@yakimant, I agree, and I was anticipating this kind of comment in this PR. Since you rejected the PR I will refactor. Thanks for the feedback 👍🏼
[Just my opinion] I personally don't like makefiles to create interfaces for development, it's always the same story, they end up being a layer of indirection to scripts. Now, using make to help efficiently build source code is a different story, but that's not what our targets do. I worked in a few projects that used makefiles only for that purpose and the rest were all scripts following some common conventions. It was pretty nice! Eventually one project evolved to have an internal CLI, but okay, that can be overkill for us now.
There was a problem hiding this comment.
Why don't you like make for the purpose of interface for usefull scripts?
I find it useful - you just type make and you see all the options.
What would be the alternative?
You can always place your code to the scripts and call them directly if you don't want to use make.
There was a problem hiding this comment.
Thanks for asking @yakimant. First of all, I don't feel strong about replacing make, but I felt like sharing anyway. Subconsciously I probably just wanted to hear your perspective, which I do in fact :)
What would be the alternative?
The alternative could be just (https://github.com/casey/just), which is described as a command runner (which is what we use make for in status-mobile). I've been trying to use it in personal projects to get the feeling of the good and bad things. So far, it's quite smooth coming from make.
I find it useful - you just type make and you see all the options.
Do you mean completion at point? Yes, that's very useful. Interestingly, our Makefile contains cryptic code to support make help, that comes OTB with just.
Another alternative for a group of Clojure devs is to replace scripts and/or some/all make targets with babashka behind a CLI (for discoverability). This wouldn't fly well for others that don't know Clojure. It could be in another language. As I said before, it sounds overkill, but it could promote a better DX over time (which was the case in my past experience)
In my systems I don't even have make available by default, although it's an essential piece of software, nowadays I also install just.
You can always place your code to the scripts and call them directly if you don't want to use make
Yes, but then it's like I said, from my personal perspective, make becomes an unnecessary piece of software when it's used mostly as an indirection layer to scripts.
If we only used scripts and a basic CLI for discoverability (optional) it would work just as well, or at lest that's my theory :)
Makefile
Outdated
| yarn install | ||
| nodemon --exec 'yarn shadow-cljs compile mocks && yarn shadow-cljs compile test && node --require ./test-resources/override.js target/test/test.js' -e cljs | ||
| yarn shadow-cljs compile mocks && \ | ||
| nodemon --exec 'yarn shadow-cljs compile test && node --require ./test-resources/override.js target/test/test.js' -e cljs |
There was a problem hiding this comment.
Same, code duplication. Better to extract and parametrise.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks @ilmotta, I would move export TARGET := clojure it possible too.
If not - I'm ok to merge.
|
Good job! Wish status-go unit and integration tests are also split one day! |
ilmotta
force-pushed
the
split-integration-tests-from-unit-tests
branch
from
1b935a8 to
9c3459a
Compare
Makefile
Outdated
| yarn shadow-cljs compile mocks && \ | ||
| nodemon --exec "yarn shadow-cljs compile test && node --require ./test-resources/override.js $$SHADOW_OUTPUT_TO" -e cljs | ||
|
|
||
| test: export TARGET := clojure |
There was a problem hiding this comment.
What if you move this to subtasks too?
Will it work?
There was a problem hiding this comment.
Good idea and it does work locally. Pushed now, hopefully it works in the CI too.
Edit: All green ;)
ilmotta
force-pushed
the
split-integration-tests-from-unit-tests
branch
from
9c3459a to
8270947
Compare
|
@cammellos - is it possible to make this part of the CI process for Status Go afterwards? I don't mean technically as I'm sure we can figure that out, but more that we have agreement to add it? 🤔 |
|
@J-Son89, that's interesting! Do you want to run |
|
Yep exactly. Essentially we have contract tests then for the mobile client. Would be great if Desktop did the same too! 🙏 |
There was a problem hiding this comment.
I think we are creating too many targets. If you want -watch targets you can just make the normal target accept a WATCH(or some other name) parameter and then just create aliases using it:
test: WATCH ?= false
ifeq ($(WATCH), false)
some thing
else
some other thing
endif
test-watch: WATCH := true
test-watch: test| nodemon --exec 'yarn shadow-cljs compile mocks && yarn shadow-cljs compile test && node --require ./test-resources/override.js target/test/test.js' -e cljs | ||
| _clojure-test: export TARGET := clojure | ||
| _clojure-test: | ||
| yarn install && \ |
There was a problem hiding this comment.
Why does this start with underscore? And it's missing a help message.
There was a problem hiding this comment.
And the name should be test-clojure if we want to have consistent prefixes.
There was a problem hiding this comment.
Why does this start with underscore? And it's missing a help message.
There are other targets prefixed with underscore, so I used the same idea to mean "private" targets that shouldn't be used directly. Edit: Being "private", I preferred not to document them, but I can add something no problem.
And the name should be test-clojure if we want to have consistent prefixes.
Good point, will do.
There was a problem hiding this comment.
Resolved by bcf0af0
I kept the underscore prefix like other targets and I didn't add any documentation. The "public" targets have a description though. Is that okay for you?
Makefile
Outdated
| _clojure-test-watch: export TARGET := clojure | ||
| _clojure-test-watch: |
There was a problem hiding this comment.
Why does this start with underscore? And it's missing a help message.
There was a problem hiding this comment.
And should be called test-clojure-watch or similar to have the same prefix as other test targets.
There was a problem hiding this comment.
Also, why do we need a separate target? Why can't you just have a variable that decides if it's a watch version or not?
There was a problem hiding this comment.
I think it's a matter of convenience @jakubgs. Passing parameters to make is not as convenient as using the auto-complete of available targets and just pressing tab and enter. Obviously, this leads to a bit of code duplication.
We can have a variable for sure, a matter of taste :)
There was a problem hiding this comment.
Resolved by bcf0af0
Now the dev must pass WATCH=true to the test targets they care about watching files and the Makefile is more DRY now.
Also renamed the targets to be more consistent. I did not rename the component test targets. I consider that out of scope for this PR.
@jakubgs, unfortunately the code you suggested doesn't work. The conditional Here's an example: foo: export TARGET := clojure
foo: export WATCH ?= false
foo:
ifeq ($(WATCH), true)
@echo watch=true
else
@echo watch=false
endif
bar: export WATCH := true
bar: foomake foo # => prints watch=false
make bar # => prints watch=false
make foo WATCH=true # => prints watch=true
make bar WATCH=true # => prints watch=trueIf we change to recursive make call it works. bar:
$(MAKE) foo WATCH=trueSo as we can see the variable doesn't propagate. Therefore, the solution is to either drop these targets |
ilmotta
force-pushed
the
split-integration-tests-from-unit-tests
branch
from
8270947 to
bcf0af0
Compare
There was a problem hiding this comment.
@ilmotta thanks for testing that. That is indeed annoying, make is such an obtuse tool to use.
I guess adding a test-watch target that uses the $(MAKE) trick is fine for the sake of discoverability of such command in the make help message.
ilmotta
force-pushed
the
split-integration-tests-from-unit-tests
branch
from
bcf0af0 to
3c19360
Compare
81% of end-end tests have passedFailed tests (7)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (39)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
ilmotta
force-pushed
the
split-integration-tests-from-unit-tests
branch
from
3c19360 to
b2ad9e3
Compare
- make test still exists, so if you have been using it, as well as make test-watch, they should all work exactly the same. - [Changed] As part of the check stage, Jenkins will run Lint and Unit Tests in parallel. Integration Tests run later because running them at the same time as Unit Tests caused errors. - [Added] "make unit-test" and "make unit-test-watch" run unit tests only. Watching all unit tests is faster now because we ignore integration tests and we only compile shadow-cljs :mock target once. We are at approximately 10-15s to re-run all unit tests after saving a watched file, depending on your hardware. If you change mocks.js_dependencies.cljs, you must re-run the make target. - [Added] "make integration-test" and "make integration-test-watch" run integration tests only. These are much slower than the unit tests. Fixes #18112
Fixes #18112
Summary
Run Clojure integration tests separately from unit tests in the CI.
Why?
make test-watch, which re-runs the slow integration tests alongside unit tests. The dev can control the namespace regex that decides which test namespaces to run, but there's no easy way to watch only unit tests or only integration tests.Changelog:
make teststill exists, so if you have been using it, as well asmake test-watch, they should all work exactly the same.[Changed]As part of the check stage, Jenkins will runLintandUnit Testsin parallel.Integration Testsrun later because running them at the same time asUnit Testscaused errors (see https://ci.status.im/blue/organizations/jenkins/status-mobile%2Fprs%2Ftests/detail/PR-18304/2/pipeline).[Added]make unit-testandmake unit-test-watchrun unit tests only. Watching all unit tests is faster now because we ignore integration tests and we only compile shadow-cljs:mocktarget once. We are at approximately 10-15s to re-run all unit tests after saving a watched file, depending on your hardware. If you changemocks.js_dependencies.cljs, you must re-run the make target.[Added]make integration-testandmake integration-test-watchrun integration tests only. These are much slower than the unit tests.Original idea from @J-Son89 👍
Areas that may be impacted
Most definitely nothing for production code.
Steps to test
Of course the Jenkins output since we're changing the pipeline, but you can also run the new make targets locally to check.
status: ready