Control lightweight storage integration tests via build tags #3346

rbroggi · 2021-10-28T17:57:44Z

introduce build tags grpc_storage_integration, badger_storage_integration, and memory_storage_integration for better control over badgerstore_test.go and grpc_test.go
run memory storage test as part of make-test, and run badger/grpc in a separate CI workflow

Which problem is this PR solving?

Part of Use integration tests to generate coverage #1516

Short description of the changes

Integration of two build tags that enable ease control of when badge and grpc_plugin tests must be run

rbroggi · 2021-10-28T18:03:50Z

Hello all, this is my first time around :)

I'm a Jaeger user and a fan of the product. Being part somehow of the project is something that will make me very proud.

Two days ago I also discovered the hacktoberfest initiative and was very happy to find Jaeger issues there.

I hope we make it on time to make this PR ready before time is up for hacktoberfest but if it will not be possible, no problem at all :)

I wish you a pleasant day/evening

yurishkuro · 2021-10-28T23:02:40Z

examples/memstore-plugin/main.go

@@ -11,6 +11,8 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
+//go:build grpc_plugin_storage_integration
+// +build grpc_plugin_storage_integration


why does this need to be conditional? this only builds the binary when requested

It indeed does not need to be conditional. If you prefer I can remove it. It was only a way to 'link' the execution of the grpc_test.go. I understand also that this file has no need to be compiled in other scopes, is this right? Anyhow for me there is no problem if you prefer to eliminate the compilation tags for this file.

I would remove the tag. Someone needs to explicitly invoke the build of this module, so having an extra protection via tag just adds more friction.

Makefile

* introduction of build tags [grpc_plugin_storage_integration, badger_storage_integration] for better control over badgerstore_test.go and grpc_test.go which being fairly light-weight can be run along with the main make target `test` Signed-off-by: rbroggi <[email protected]>

Signed-off-by: rbroggi <[email protected]>

codecov · 2021-10-29T16:31:18Z

Codecov Report

Merging #3346 (fa791a0) into master (77e5674) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3346      +/-   ##
==========================================
- Coverage   96.48%   96.46%   -0.02%     
==========================================
  Files         259      259              
  Lines       15166    15166              
==========================================
- Hits        14633    14630       -3     
- Misses        450      452       +2     
- Partials       83       84       +1

Impacted Files	Coverage Δ
cmd/query/app/static_handler.go	`97.00% <0.00%> (-1.80%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77e5674...fa791a0. Read the comment docs.

yurishkuro · 2021-10-29T16:58:40Z

.github/workflows/ci-memory-badger.yml

@@ -1,20 +0,0 @@
-name: CIT Memory And Badger


why are you removing this workflow?

I thought to remove it because the only thing it did was to run make mem-and-badger-storage-integration-test and now the whole suite included in make mem-and-badger-storage-integration-test is actually run along with the target test. Makes sense? I thought it was redundant, but I might have misunderstood.

makes sense.

Actually, one reason why I wanted to have build tags was so that memory/badger storage tests would not need to run when we run ES/Cassandra tests. But they still seem to run per https://github.com/jaegertracing/jaeger/runs/4048978413?check_suite_focus=true

I agree they should not run. Let me check

I simply did not use a build-tag for those tests.

Please let me know if you prefer to have a unique tag for all the 3 tests (grpc/badge/memory) instead of one for each

As promissed:

https://github.com/jaegertracing/jaeger/runs/4050387504?check_suite_focus=true

rbroggi · 2021-10-29T17:09:49Z

Is CIT Elasticsearch / opensearch 1.x (pull_request) expected to fail?

Nothing has been touched in the scope of those tests...

Signed-off-by: rbroggi <[email protected]>

rbroggi · 2021-10-29T20:04:21Z

Codecov Report

Merging #3346 (95675b3) into master (8b7772e) will decrease coverage by 1.41%.
The diff coverage is n/a.
@@            Coverage Diff             @@
##           master    #3346      +/-   ##
==========================================
- Coverage   96.01%   94.60%   -1.42%     
==========================================
  Files         261      261              
  Lines       15464    15464              
==========================================
- Hits        14848    14629     -219     
- Misses        523      751     +228     
+ Partials       93       84       -9     
Impacted Files Coverage Δ
plugin/storage/integration/integration.go 0.00% <0.00%> (-79.29%)
plugin/storage/integration/trace_compare.go 0.00% <0.00%> (-36.18%)
cmd/query/app/server.go 94.11% <0.00%> (-1.48%)
plugin/storage/badger/spanstore/reader.go 95.50% <0.00%> (-0.71%)
cmd/query/app/static_handler.go 97.60% <0.00%> (+1.79%)
Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b7772e...95675b3. Read the comment docs.

I'm surprised about the outcome 🤔

Do you think we were doubly counting anything on the coverage?

yurishkuro · 2021-10-30T01:31:42Z

I doubt it, coverage is reported with the identity of lines of code, so codecov will merge them, not double count.

rbroggi · 2021-10-30T10:39:47Z

Codecov Report

Merging #3346 (95675b3) into master (8b7772e) will decrease coverage by 1.41%.
The diff coverage is n/a.
@@            Coverage Diff             @@
##           master    #3346      +/-   ##
==========================================
- Coverage   96.01%   94.60%   -1.42%     
==========================================
  Files         261      261              
  Lines       15464    15464              
==========================================
- Hits        14848    14629     -219     
- Misses        523      751     +228     
+ Partials       93       84       -9     
Impacted Files Coverage Δ
plugin/storage/integration/integration.go 0.00% <0.00%> (-79.29%)
plugin/storage/integration/trace_compare.go 0.00% <0.00%> (-36.18%)
cmd/query/app/server.go 94.11% <0.00%> (-1.48%)
plugin/storage/badger/spanstore/reader.go 95.50% <0.00%> (-0.71%)
cmd/query/app/static_handler.go 97.60% <0.00%> (+1.79%)
Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b7772e...95675b3. Read the comment docs.
I'm surprised about the outcome thinking

Do you think we were doubly counting anything on the coverage?

After checking better the report here is what I found:

The gap in coverability comes massively from plugin/storage/integration/integration.go and plugin/storage/integration/trace_compare.go which, by their content, sound to me as test-utility functions, and therefore I think they would be perfectly eligible to be excluded from coverability altogether. WDYT?

We can access the exclusion of those files in a different PR. I think I understood the source of the issue:

The new tags were not used in the cover make target. I have updated the PR.

Signed-off-by: rbroggi <[email protected]>

…ation,memory_storage_integration` should be present in `cover` profile and in `test` profile. Signed-off-by: rbroggi <[email protected]>

Signed-off-by: rbroggi <[email protected]>

yurishkuro · 2021-10-30T14:37:04Z

Unit tests fail on gRPC plugin integration tests. I would not necessarily consider it a "cheap" storage since it requires building the plugin binary first, so not a good candidate for quick make test.

My recommendation is to only keep memory test as "cheap" (and therefore not use the tag "cheap*" but explicitly memory_storage_integration), and do grpc + badger as a standalone GH Actions workflow as we had before.

rbroggi · 2021-10-30T14:39:22Z

Unit tests fail on gRPC plugin integration tests. I would not necessarily consider it a "cheap" storage since it requires building the plugin binary first, so not a good candidate for quick make test.

My recommendation is to only keep memory test as "cheap" (and therefore not use the tag "cheap*" but explicitly memory_storage_integration), and do grpc + badger as a standalone GH Actions workflow as we had before.

single tag for grpc+ badger? or prefer to keep 2 distinct ?

* including memory-integration-storage as a cheap integration test to run along with `make test` * adapt the name of the ci workflow Signed-off-by: rbroggi <[email protected]>

…torage_integration Signed-off-by: rbroggi <[email protected]>

…ion-test` Signed-off-by: rbroggi <[email protected]>

Signed-off-by: rbroggi <[email protected]>

Signed-off-by: Yuri Shkuro <[email protected]>

rbroggi requested a review from a team as a code owner October 28, 2021 17:57

rbroggi requested a review from pavolloffay October 28, 2021 17:57

rbroggi force-pushed the 1516-storage-integration-tests branch from 7af3a5c to df4cccc Compare October 28, 2021 17:59

yurishkuro reviewed Oct 28, 2021

View reviewed changes

rbroggi added 2 commits October 29, 2021 18:30

eliminate the build tag from the binary compilation

c63d9ae

Signed-off-by: rbroggi <[email protected]>

rbroggi force-pushed the 1516-storage-integration-tests branch from 029ffcf to c63d9ae Compare October 29, 2021 16:30

yurishkuro changed the title ~~Issue #1516 https://github.com/jaegertracing/jaeger/issues/1516~~ Control storage integration tests via build tags Oct 29, 2021

yurishkuro reviewed Oct 29, 2021

View reviewed changes

Including memstore as test to be run as part of regular tests

95675b3

Signed-off-by: rbroggi <[email protected]>

rbroggi force-pushed the 1516-storage-integration-tests branch from 230e931 to 95675b3 Compare October 29, 2021 19:11

rbroggi added 3 commits October 30, 2021 12:53

Coverability decreases due to missing tags in cover make target

352bda3

Signed-off-by: rbroggi <[email protected]>

New build tags `badger_storage_integration,grpc_plugin_storage_integr…

d0411c8

…ation,memory_storage_integration` should be present in `cover` profile and in `test` profile. Signed-off-by: rbroggi <[email protected]>

unifying the build-tags of cheap integration tests

9721d17

Signed-off-by: rbroggi <[email protected]>

rbroggi added 4 commits October 30, 2021 16:49

* bundling grpc and badger tests into single tag for execution

f519277

* including memory-integration-storage as a cheap integration test to run along with `make test` * adapt the name of the ci workflow Signed-off-by: rbroggi <[email protected]>

* changing compilation tag from cheap_storage_integration to memory_s…

b927512

…torage_integration Signed-off-by: rbroggi <[email protected]>

Avoid running all unit-tests in the `grpc-and-badger-storage-integrat…

f3f8ce7

…ion-test` Signed-off-by: rbroggi <[email protected]>

renaming the github action step

9cc21d8

Signed-off-by: rbroggi <[email protected]>

rbroggi requested a review from yurishkuro October 30, 2021 19:13

yurishkuro added 2 commits October 30, 2021 17:17

use independent tags

bf916f3

Signed-off-by: Yuri Shkuro <[email protected]>

split grpc/badger more

569d5bd

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro approved these changes Oct 30, 2021

View reviewed changes

yurishkuro changed the title ~~Control storage integration tests via build tags~~ Control lightweight storage integration tests via build tags Oct 30, 2021

yurishkuro enabled auto-merge (squash) October 30, 2021 21:24

Merge branch 'master' into 1516-storage-integration-tests

fa791a0

yurishkuro merged commit 7844198 into jaegertracing:master Oct 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control lightweight storage integration tests via build tags #3346

Control lightweight storage integration tests via build tags #3346

rbroggi commented Oct 28, 2021 •

edited by yurishkuro

Loading

rbroggi commented Oct 28, 2021 •

edited

Loading

yurishkuro Oct 28, 2021

rbroggi Oct 29, 2021

yurishkuro Oct 29, 2021 •

edited

Loading

rbroggi Oct 29, 2021

codecov bot commented Oct 29, 2021 •

edited

Loading

yurishkuro Oct 29, 2021

rbroggi Oct 29, 2021

yurishkuro Oct 29, 2021

yurishkuro Oct 29, 2021

rbroggi Oct 29, 2021

rbroggi Oct 29, 2021

rbroggi Oct 29, 2021

rbroggi commented Oct 29, 2021

rbroggi commented Oct 29, 2021

Codecov Report

yurishkuro commented Oct 30, 2021

rbroggi commented Oct 30, 2021 •

edited

Loading

Codecov Report

yurishkuro commented Oct 30, 2021

rbroggi commented Oct 30, 2021

Control lightweight storage integration tests via build tags #3346

Control lightweight storage integration tests via build tags #3346

Conversation

rbroggi commented Oct 28, 2021 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

rbroggi commented Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbroggi commented Oct 29, 2021

rbroggi commented Oct 29, 2021

Codecov Report

yurishkuro commented Oct 30, 2021

rbroggi commented Oct 30, 2021 • edited Loading

Codecov Report

yurishkuro commented Oct 30, 2021

rbroggi commented Oct 30, 2021

rbroggi commented Oct 28, 2021 •

edited by yurishkuro

Loading

rbroggi commented Oct 28, 2021 •

edited

Loading

yurishkuro Oct 29, 2021 •

edited

Loading

codecov bot commented Oct 29, 2021 •

edited

Loading

rbroggi commented Oct 30, 2021 •

edited

Loading