Rewrite specs using rspec #135

kbogtob · 2020-02-25T18:35:56Z

What does this PR?

It rewrites the tests using RSpec and mocking in the idea of refactoring the project to have more SOLID code. Some tests are still breaking encapsulation and fixing them implies to modify the code to make it more testable and uncoupled.

Motivation

Clarify the project, use more recent testing techniques and allowing people to contribute easily + making grounds to refactor the project.

Notes

Found a bug, notified it in comment.

kbogtob · 2020-02-26T14:29:06Z

lib/datadog/statsd/uds_connection.rb

@@ -28,6 +28,9 @@ def send_message(message)
        socket.sendmsg_nonblock(message)
      rescue Errno::ECONNREFUSED, Errno::ECONNRESET, Errno::ENOENT => e
        @socket = nil
+        # TODO: FIXME: This error should be considered as a retryable error in the


I found this bug, it was not caught by the previous tests: If we catch a connection refused or reset, we don't try to reconnect to the UDS socket. For the missing socket, it seems relevant, it means that the socket was never found, but for other cases, I think we should retry.

albertvaka

Do you think it could be worth keeping some of the old tests that were more like integration/high level? Aren't we reducing the coverage?

albertvaka · 2020-03-16T12:38:38Z

spec/matchers/telemetry_matcher.rb

+RSpec::Matchers.define :eq_with_telemetry do |expected, telemetry_options|
+  telemetry_options ||= {}
+
+  def text_with_telemetry(text, metrics: 1, events: 0, service_checks: 0, bytes_sent: 0, bytes_dropped:0, packets_sent: 0, packets_dropped: 0, transport: 'udp')


Suggested change

def text_with_telemetry(text, metrics: 1, events: 0, service_checks: 0, bytes_sent: 0, bytes_dropped:0, packets_sent: 0, packets_dropped: 0, transport: 'udp')

# Appends the telemetry metrics to the metrics string passed as 'text'

def add_telemetry(text, metrics: 1, events: 0, service_checks: 0, bytes_sent: 0, bytes_dropped:0, packets_sent: 0, packets_dropped: 0, transport: 'udp')

Better function name (I think).

albertvaka · 2020-03-16T12:38:52Z

spec/matchers/telemetry_matcher.rb

+  end
+
+  match do |actual|
+    actual == text_with_telemetry(expected, **telemetry_options)


Suggested change

actual == text_with_telemetry(expected, **telemetry_options)

actual == add_telemetry(expected, **telemetry_options)

albertvaka · 2020-03-16T15:50:41Z

spec/statsd/version_spec.rb

+describe Datadog::Statsd do
+  describe 'VERSION' do
+    it 'has a version' do
+      expect(Datadog::Statsd::VERSION).to eq '4.6.0'


We will need to update this for every version. Isn't it better to use a regex like before?

I like this part to be explicit everytime we release but I can change it to a regexp

Let's properly update the release documentation then, let me know if you need the link to the document I'm mentioning

albertvaka · 2020-03-16T15:58:24Z

spec/integrations/allocation_spec.rb

+    end
+
+    let(:expected_allocations) do
+      if RUBY_VERSION >= '2.3.0' && RUBY_VERSION < '2.4.0'


Suggested change

if RUBY_VERSION >= '2.3.0' && RUBY_VERSION < '2.4.0'

if RUBY_VERSION < '2.4.0'

Otherwise, older rubies would fall in the else case, which isn't what this code wants to express.

It is what I wanted :). Older rubies make 8 allocations in this case.

albertvaka · 2020-03-16T15:58:44Z

spec/integrations/allocation_spec.rb

+  end
+
+  let(:expected_allocations) do
+    if RUBY_VERSION >= '2.3.0' && RUBY_VERSION < '2.4.0'


Suggested change

if RUBY_VERSION >= '2.3.0' && RUBY_VERSION < '2.4.0'

if RUBY_VERSION < '2.4.0'

idem.

It is what I wanted :). Older rubies make 8 allocations in this case.

remeh

Did my best to review this carefully but I won't lie, I may have been a bit fast on some of the tests.
In all cases, great update and it'll definitely be helpful to have solid unit testing on this lib. Thanks 🙇

remeh · 2020-03-17T15:21:22Z

lib/datadog/statsd/version.rb

+
+module Datadog
+  class Statsd
+    VERSION = '4.6.0'


Nit: it's most likely 4.7.0 now.

I'll fix it :)

remeh · 2020-03-17T15:22:24Z

spec/statsd/version_spec.rb

+describe Datadog::Statsd do
+  describe 'VERSION' do
+    it 'has a version' do
+      expect(Datadog::Statsd::VERSION).to eq '4.6.0'


Let's properly update the release documentation then, let me know if you need the link to the document I'm mentioning

remeh · 2020-03-17T15:32:18Z

spec/integrations/connection_edge_case_spec.rb

+        context 'because of an unknown error' do
+          before do
+            allow(fake_socket_retry)
+              .to receive(:send)
+              .and_raise(RuntimeError, 'yolo')
+          end
+
+          it 'ignores the connection failure' do
+            expect do
+              subject.write('foobar')
+            end.not_to raise_error
+          end
+
+          it 'logs the error message' do
+            subject.write('foobar')
+            expect(log.string).to match 'Statsd: RuntimeError yolo'
+          end
+        end
+
+        context 'because of a SocketError' do
+          before do
+            allow(fake_socket_retry)
+              .to receive(:send)
+              .and_raise(SocketError, 'yolo')
+          end
+
+          it 'ignores the connection failure' do
+            expect do
+              subject.write('foobar')
+            end.not_to raise_error
+          end
+
+          it 'logs the error message' do
+            subject.write('foobar')
+            expect(log.string).to match 'Statsd: SocketError yolo'
+          end
+        end
+      end


Aren't they testing the same thing? I'm may miss a subtlety here.

It is, just to be sure that we don't handle socketerrors differently.

remeh · 2020-03-17T15:36:06Z

spec/integrations/live_testing_spec.rb

+      statsd = Datadog::Statsd.new(host, port)
+      statsd.increment('foobar')
+      message = socket.recvfrom(64).first
+      expect(message).to eq 'foobar:1|c'


Should we test more than just a counter? Probably not has you improved the test coverage on everything but if that's like 2, 4 more lines, could be helpful someday.

We could make a big test for that. I'll add it.

remeh · 2020-03-17T15:38:38Z

spec/statsd_spec.rb

  let(:sample_rate) { nil }
-  let(:socket) { FakeUDPSocket.new }
+  let(:tags) { %w[abc def] }


Because it doesn't harm, let's use tags just slightly more realistic: for instance abc:def host:myhost12-11, WDYT?

Actually you did it right after, nvm.

We discovered that there is a bug with which strategy each error has to have. Connection refused or connection reset are not retried on UDS Connections.

kbogtob added the WIP label Feb 25, 2020

kbogtob force-pushed the kbogtob/rewrite-specs branch from 878b808 to 179f9e6 Compare February 26, 2020 12:08

kbogtob changed the title ~~(WIP) Rewrite specs using rspec~~ Rewrite specs using rspec Feb 26, 2020

kbogtob removed the WIP label Feb 26, 2020

kbogtob force-pushed the kbogtob/rewrite-specs branch 2 times, most recently from e3eb299 to 7c1e15d Compare February 26, 2020 14:26

kbogtob commented Feb 26, 2020

View reviewed changes

kbogtob force-pushed the kbogtob/rewrite-specs branch 2 times, most recently from 30907e1 to 9603e9f Compare February 26, 2020 15:26

kbogtob requested review from remeh and albertvaka February 26, 2020 15:30

albertvaka reviewed Mar 16, 2020

View reviewed changes

kbogtob force-pushed the kbogtob/rewrite-specs branch from 9603e9f to 4cbbd8e Compare March 17, 2020 12:21

albertvaka approved these changes Mar 17, 2020

View reviewed changes

remeh approved these changes Mar 17, 2020

View reviewed changes

kbogtob added 8 commits March 23, 2020 09:57

Move minitests to test folder

fba4e56

Remove useless dependencies

6953645

Add basic specs for gem version

d4d8ba5

Add statsd #initialize rspec tests

2886378

Add metrics methods specs

194bbab

Add specs for the #event and #service_check methods

d856032

Add specs for #batch and #close

622b445

Add allocation specs

dd9b18a

kbogtob force-pushed the kbogtob/rewrite-specs branch from 4cbbd8e to 3d6e252 Compare March 23, 2020 17:34

kbogtob added 4 commits March 23, 2020 18:41

Add live-testing tests

dee885c

Fix telemetry specs by breaking encapsulation

f2d7d5b

Add connection edge case integration tests

41291ec

We discovered that there is a bug with which strategy each error has to have. Connection refused or connection reset are not retried on UDS Connections.

Remove monolithic minitest tests and setup CI

f698c8d

kbogtob force-pushed the kbogtob/rewrite-specs branch from 3d6e252 to f698c8d Compare March 23, 2020 17:42

albertvaka approved these changes Mar 23, 2020

View reviewed changes

kbogtob merged commit 040407d into master Mar 23, 2020

kbogtob deleted the kbogtob/rewrite-specs branch April 7, 2020 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite specs using rspec #135

Rewrite specs using rspec #135

kbogtob commented Feb 25, 2020 •

edited

Loading

kbogtob Feb 26, 2020

albertvaka left a comment

albertvaka Mar 16, 2020

kbogtob Mar 17, 2020

albertvaka Mar 16, 2020

albertvaka Mar 16, 2020

kbogtob Mar 17, 2020

remeh Mar 17, 2020

albertvaka Mar 16, 2020

kbogtob Mar 17, 2020

albertvaka Mar 16, 2020

kbogtob Mar 17, 2020

remeh left a comment

remeh Mar 17, 2020

kbogtob Mar 23, 2020

remeh Mar 17, 2020

remeh Mar 17, 2020

kbogtob Mar 23, 2020

remeh Mar 17, 2020

kbogtob Mar 23, 2020

remeh Mar 17, 2020

remeh Mar 17, 2020

	def text_with_telemetry(text, metrics: 1, events: 0, service_checks: 0, bytes_sent: 0, bytes_dropped:0, packets_sent: 0, packets_dropped: 0, transport: 'udp')
	# Appends the telemetry metrics to the metrics string passed as 'text'
	def add_telemetry(text, metrics: 1, events: 0, service_checks: 0, bytes_sent: 0, bytes_dropped:0, packets_sent: 0, packets_dropped: 0, transport: 'udp')

	actual == text_with_telemetry(expected, **telemetry_options)
	actual == add_telemetry(expected, **telemetry_options)

	if RUBY_VERSION >= '2.3.0' && RUBY_VERSION < '2.4.0'
	if RUBY_VERSION < '2.4.0'

Rewrite specs using rspec #135

Rewrite specs using rspec #135

Conversation

kbogtob commented Feb 25, 2020 • edited Loading

What does this PR?

Motivation

Notes

Choose a reason for hiding this comment

albertvaka left a 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

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

remeh left a 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

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

kbogtob commented Feb 25, 2020 •

edited

Loading