Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add integration tests for async/amqp, trace/amqp #172

Closed
wants to merge 39 commits into from

Conversation

tpaschalis
Copy link

@tpaschalis tpaschalis commented Mar 7, 2020

Which problem is this PR solving?

This PR closes #171 Add integration tests for async/amqp.

Short description of the changes

  • Modify Github Actions definition so that it starts the rabbitmq container and network.
  • Add integration tests for the async/amqp package.
  • Add integration tests for the trace/amqp package.

Notes

  • I'm using different queues for each package, as I came across messages from one package 'leaking' into another package's tests.

  • I will have to run some commits so I can live-debug Github Actions, hope it's okay.

As we mentioned, we need to avoid flakiness.
The tests seem to work without flakiness both locally and on Github Actions.

$ for i in {1..50}; do
go test -timeout 30s github.com/beatlabs/patron/async/amqp -run "^(TestConsumeAndDeliver)$" -count=1
done | grep "^ok" | wc -l
    50 
$ for i in {1..20}; do go test ./component/async/amqp -tags=integration -count=1; done | grep ^ok | wc -l
      20
$ for i in {1..20}; do go test ./client/amqp -tags=integration -count=1; done | grep ^ok | wc -l
      20

@mantzas
Copy link

mantzas commented Mar 8, 2020

@tpaschalis Hi, we have to support integration tests to run locally also so we should start to use docker-compose for spinning up all necessary instances.
Github actions seem to be supporting this.

@tpaschalis
Copy link
Author

tpaschalis commented Mar 8, 2020

@tpaschalis Hi, we have to support integration tests to run locally also so we should start to use docker-compose for spinning up all necessary instances.
Github actions seem to be supporting this.

This PR uses Github Actions to spin up the Rabbit MQ container we already have in our examples, and it seems to be working okay. I did have some problems with flakiness which were solved; nevertheless we should keep this in mind.

For running tests locally, it might be a good idea to separate unit and integration tests, so the suite can run faster, and without external dependencies.

This could happen either with a flag

var runIntegrationTests = flag.Bool("integration", false
    , "Run the integration tests (in addition to the unit tests)")

func TestXYZIntegration(t *testing.T) {
    if !*runIntegrationTests {
        this.T().Skip("")
    }
    ...
}

using build constraints by having separate XYZ_integration_test.go test files, runnable with go test -v -tags integration, by including a top-line //+build integration,

or using the built-in short flag where we would

    if testing.Short() {
        t.Skip("skipping integration tests in short mode")
    }

What do you think?

@mantzas
Copy link

mantzas commented Mar 8, 2020

@tpaschalis We have already a way of working with unit and integrations tests established in the company by using build tags. Please wait for issue #142 to be merged in order to build upon that.
This way we support euqally local and GitHub test runs.

@tpaschalis tpaschalis changed the title Extend async/amqp testsuite WIP Extend async/amqp testsuite Mar 8, 2020
@tpaschalis tpaschalis changed the title WIP Extend async/amqp testsuite WIP async/amqp integration tests Mar 8, 2020
@mantzas
Copy link

mantzas commented Mar 10, 2020

@tpaschalis We have now in master integration tests. You can use the docker-compose file to add rabbitmq and run these tests.

@tpaschalis tpaschalis force-pushed the extend-amqp-testsuite branch 2 times, most recently from ce54bc0 to 59c708d Compare March 10, 2020 07:54
@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #172 into master will decrease coverage by 10.32%.
The diff coverage is 53.35%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #172       +/-   ##
===========================================
- Coverage   84.48%   74.15%   -10.33%     
===========================================
  Files          52       57        +5     
  Lines        3074     3235      +161     
===========================================
- Hits         2597     2399      -198     
- Misses        398      769      +371     
+ Partials       79       67       -12     
Impacted Files Coverage Δ
client/amqp/amqp.go 40.98% <ø> (ø)
client/amqp/option.go 66.66% <ø> (ø)
client/amqp/user.pb.go 26.92% <ø> (ø)
client/http/option.go 100.00% <ø> (ø)
client/kafka/async_producer.go 0.00% <0.00%> (ø)
client/kafka/sync_producer.go 0.00% <0.00%> (ø)
client/redis/redis.go 44.44% <ø> (ø)
client/sns/message.go 97.53% <ø> (ø)
client/sns/publisher.go 93.33% <ø> (ø)
client/sql/sql.go 17.53% <ø> (-68.73%) ⬇️
... and 73 more

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 d51c80b...cd35caa. Read the comment docs.

@tpaschalis tpaschalis changed the title WIP async/amqp integration tests Add integration tests for async/amqp, trace/amqp Mar 12, 2020
trace/amqp/integration_test.go Outdated Show resolved Hide resolved
trace/amqp/integration_test.go Outdated Show resolved Hide resolved
async/amqp/integration_test.go Outdated Show resolved Hide resolved
@tpaschalis tpaschalis force-pushed the extend-amqp-testsuite branch 2 times, most recently from 50ebfee to 9fe3c2a Compare March 16, 2020 11:15
@tpaschalis tpaschalis requested a review from mantzas March 16, 2020 13:55
async/amqp/integration_test.go Outdated Show resolved Hide resolved
trace/amqp/integration_test.go Outdated Show resolved Hide resolved
trace/amqp/integration_test.go Outdated Show resolved Hide resolved
trace/amqp/integration_test.go Outdated Show resolved Hide resolved
Paschalis Tsilias added 15 commits June 24, 2020 11:45
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Paschalis Tsilias added 2 commits June 24, 2020 12:19
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
…alid exchange

Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Paschalis Tsilias added 5 commits June 24, 2020 12:26
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
@tpaschalis
Copy link
Author

I'll be closing this PR as it's been too long to even remember where I left it at.

I'll take a stab into writing the integration tests from scratch using dockertest like the other integration tests do.

@tpaschalis tpaschalis closed this Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add integration tests for async/amqp
4 participants