Split Go and React Tests#8897
Conversation
|
Somehow only some tests did run |
|
Yes, this is because I had CircleCI on in my repository, so this PR ran under my installation and messed everything up. |
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
fe3080a to
62d28dd
Compare
Signed-off-by: Levi Harrison <git@leviharrison.dev>
|
I changed |
Makefile
Outdated
| ifeq ($(MAKECMDGOALS),go-only) | ||
| test: common-test | ||
| else | ||
| test: common-test react-app-test | ||
| endif | ||
|
|
||
| .PHONY: go-only | ||
| go-only: all |
There was a problem hiding this comment.
| ifeq ($(MAKECMDGOALS),go-only) | |
| test: common-test | |
| else | |
| test: common-test react-app-test | |
| endif | |
| .PHONY: go-only | |
| go-only: all | |
| ifeq ($(GO_ONLY),) | |
| test: common-test react-app-test | |
| else | |
| test: common-test | |
| endif |
and then run make GO_ONLY=1
that seems cleaner to me
Signed-off-by: Levi Harrison <git@leviharrison.dev> Co-authored-by: Julien Pivotto <roidelapluie@inuits.eu>
Makefile
Outdated
| cd $(REACT_APP_PATH) && yarn test --no-watch --coverage | ||
|
|
||
| .PHONY: test | ||
| test: |
There was a problem hiding this comment.
This blank one wasn't needed.
| # If we only want to only test go code we have to change the test target | ||
| # which is called by all. | ||
| ifeq ($(GO_ONLY),1) | ||
| test: common-test | ||
| else | ||
| test: common-test react-app-test | ||
| endif | ||
|
|
There was a problem hiding this comment.
Rather than complicating things like this, split the test target.
| # If we only want to only test go code we have to change the test target | |
| # which is called by all. | |
| ifeq ($(GO_ONLY),1) | |
| test: common-test | |
| else | |
| test: common-test react-app-test | |
| endif | |
| test: go-test react-app-test | |
| .PHONY: go-test | |
| go-test: common-test |
Then in CI, you can call make go-test or make react-app-test.
There was a problem hiding this comment.
Common-test only runs go test.
We need make all: precheck style check_license lint yamllint unused build test.
But make all requires test. We want it to test react locally, but only go in CI, hence the GO_ONLY for CI.
There was a problem hiding this comment.
So, we should change the make to not use the default all target. We expand what we want into separate steps.
There was a problem hiding this comment.
Do we though? That just adds more bloat to the Makefiles:
# Makefile
go-test: ...
test-go: precheck style check_license lint yamllint unused build go-test# Makefile.common
all: precheck style check_license lint yamllint unused build testThe idea of the all target is to generalize common tasks, maintaining an additional list of targets mostly the same is an annoyance. In my opinion, I think the current way is a lot easier to understand. It's just a condition that causes only go tests to run when test is called, as opposed to a whole new target which does mostly the same thing as the all target, except for the last step.
Running all on test_go follows a decision to classify the general tasks as part of Go, most of which are, but some (yamllint) don't exactly fit into that category. We may want to consider renaming the CircleCI job to just be test as it does run things that aren't strictly Go.
|
Thanks! I feel like this little piece of magic is the best option for the other repositories. |
* Fix `kuma_sd` targetgroup reporting (#9157) * Bundle all xDS targets into a single group Signed-off-by: austin ce <austin.cawley@gmail.com> * Snapshot in-memory chunks on shutdown for faster restarts (#7229) Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com> * Rename links Signed-off-by: Levi Harrison <git@leviharrison.dev> * Remove Individual Data Type Caps in Per-shard Buffering for Remote Write (#8921) * Moved everything to nPending buffer Signed-off-by: Levi Harrison <git@leviharrison.dev> * Simplify exemplar capacity addition Signed-off-by: Levi Harrison <git@leviharrison.dev> * Added pre-allocation Signed-off-by: Levi Harrison <git@leviharrison.dev> * Don't allocate if not sending exemplars Signed-off-by: Levi Harrison <git@leviharrison.dev> * Avoid deadlock when processing duplicate series record (#9170) * Avoid deadlock when processing duplicate series record `processWALSamples()` needs to be able to send on its output channel before it can read the input channel, so reads to allow this in case the output channel is full. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * processWALSamples: update comment Previous text seems to relate to an earlier implementation. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * Optimise WAL loading by removing extra map and caching min-time (#9160) * BenchmarkLoadWAL: close WAL after use So that goroutines are stopped and resources released Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * BenchmarkLoadWAL: make series IDs co-prime with #workers Series are distributed across workers by taking the modulus of the ID with the number of workers, so multiples of 100 are a poor choice. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * BenchmarkLoadWAL: simulate mmapped chunks Real Prometheus cuts chunks every 120 samples, then skips those samples when re-reading the WAL. Simulate this by creating a single mapped chunk for each series, since the max time is all the reader looks at. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * Fix comment Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * Remove series map from processWALSamples() The locks that is commented to reduce contention in are now sharded 32,000 ways, so won't be contended. Removing the map saves memory and goes just as fast. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * loadWAL: Cache the last mmapped chunk time So we can skip calling append() for samples it will reject. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * Improvements from code review Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * Full stops and capitals on comments Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * Cache max time in both places mmappedChunks is updated Including refactor to extract function `setMMappedChunks`, to reduce code duplication. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * Update head min/max time when mmapped chunks added This ensures we have the correct values if no WAL samples are added for that series. Note that `mSeries.maxTime()` was always `math.MinInt64` before, since that function doesn't consider mmapped chunks. Signed-off-by: Bryan Boreham <bjboreham@gmail.com> * Split Go and React Tests (#8897) * Added go-ci and react-ci Co-authored-by: Julien Pivotto <roidelapluie@inuits.eu> Signed-off-by: Levi Harrison <git@leviharrison.dev> * Remove search keymap from new expression editor (#9184) Signed-off-by: Julius Volz <julius.volz@gmail.com> Co-authored-by: Austin Cawley-Edwards <austin.cawley@gmail.com> Co-authored-by: Levi Harrison <git@leviharrison.dev> Co-authored-by: Julien Pivotto <roidelapluie@inuits.eu> Co-authored-by: Bryan Boreham <bjboreham@gmail.com> Co-authored-by: Julius Volz <julius.volz@gmail.com>
This PR splits the go and react tests by adding two new Makefile targets:
go-ciandreact-ci, and running separate CircleCI jobs for them. I've used a node image for the react tests, although it could be golang too. The node image does trigger these irrelevant errors, and I'm not sure why:Fixes: #8849