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

Test transaction from extension with appsignal_transaction_to_json #252

Closed
2 of 3 tasks
tombruijn opened this issue Feb 14, 2017 · 2 comments · Fixed by #1132
Closed
2 of 3 tasks

Test transaction from extension with appsignal_transaction_to_json #252

tombruijn opened this issue Feb 14, 2017 · 2 comments · Fixed by #1132
Assignees
Labels

Comments

@tombruijn
Copy link
Member

tombruijn commented Feb 14, 2017

  • Implement appsignal_transaction_to_json in the Gem's C-extension Implement Transaction#to_json #291
  • Add it to tests to check if all the assertions are correct.
  • Add test mode to extension so that we don't have to stub the complete call

See: https://github.com/appsignal/appsignal-agent/pull/228

@thijsc
Copy link
Member

thijsc commented Feb 14, 2017

appsignal_transaction_to_json can return an empty string, you should interpret that as nil. I'd return Qnil from the C glue code in that case.

@thijsc thijsc added this to the 2.2 milestone Feb 27, 2017
@tombruijn tombruijn modified the milestones: 2.3, 2.2 May 3, 2017
@tombruijn tombruijn modified the milestones: 2.4, 2.3 Jun 13, 2017
@tombruijn tombruijn changed the title Test transaction from extension with appsignal_transaction_to_json ⌨️ Test transaction from extension with appsignal_transaction_to_json Oct 27, 2017
@tombruijn tombruijn changed the title ⌨️ Test transaction from extension with appsignal_transaction_to_json Test transaction from extension with appsignal_transaction_to_json May 30, 2018
@tombruijn tombruijn removed this from the The next minor Ruby gem version milestone Jan 7, 2019
@thijsc thijsc closed this as completed Jan 20, 2021
@thijsc thijsc reopened this Jan 20, 2021
@backlog-helper
Copy link

backlog-helper bot commented Feb 4, 2021

While performing the daily checks some issues were found with this issue.

  • This issue has not had any activity in 14 days. Please provide a status update if it is still relevant. Closed it if it is no longer relevant. Or move it to another column if it's blocked or requires another look at it. - (More info)

New issue guide | Backlog management | Rules | Feedback

tombruijn added a commit that referenced this issue Feb 4, 2022
Something changed in the rspec-mocks gem between versions 3.10.2
and 3.10.3, because that's what causes tests to fail like this
on Ruby 3.1.0 and 3.0.3 locally and on the CI:

```
Failure/Error: monitor_transaction(name, env, &block)

  Appsignal received :monitor_transaction with unexpected arguments
    expected: ("perform_job.something", {:key=>:value})
         got: ("perform_job.something", {:key=>:value})
  Diff:
```

In this example the arguments are exactly the same, except they're not
the identical objects. This particular issue seems to be caused by a
recent change that for Ruby 3 keyword argument matching:
rspec/rspec-mocks#1460

## Rewriting specs

What helps is rewriting the tests to not mock method calls and check the
arguments given. Instead, assert what's being stored on the
transactions, like how we want all tests to be written as described in
issue #252.

## Adding curly brackets

For other specs that are a little more difficult to rewrite this way
right now, I've chosen to add additional curly brackets around the
argument expectations so that it's clear to Ruby/RSpec it's a hash
argument and not keywords arguments.

To solve this more permanently, we should rewrite the specs at some
point. We can remove these curly brackets if the issue is fixed in a
future rspec-mocks version as well.

[skip changeset]
tombruijn added a commit that referenced this issue Feb 7, 2022
Something changed in the rspec-mocks gem between versions 3.10.2
and 3.10.3, because that's what causes tests to fail like this
on Ruby 3.1.0 and 3.0.3 locally and on the CI:

```
Failure/Error: monitor_transaction(name, env, &block)

  Appsignal received :monitor_transaction with unexpected arguments
    expected: ("perform_job.something", {:key=>:value})
         got: ("perform_job.something", {:key=>:value})
  Diff:
```

In this example the arguments are exactly the same, except they're not
the identical objects. This particular issue seems to be caused by a
recent change that for Ruby 3 keyword argument matching:
rspec/rspec-mocks#1460

## Rewriting specs

What helps is rewriting the tests to not mock method calls and check the
arguments given. Instead, assert what's being stored on the
transactions, like how we want all tests to be written as described in
issue #252.

## Adding curly brackets

For other specs that are a little more difficult to rewrite this way
right now, I've chosen to add additional curly brackets around the
argument expectations so that it's clear to Ruby/RSpec it's a hash
argument and not keywords arguments.

To solve this more permanently, we should rewrite the specs at some
point. We can remove these curly brackets if the issue is fixed in a
future rspec-mocks version as well.

[skip changeset]
tombruijn added a commit that referenced this issue Aug 2, 2022
Instead of asserting method calls, test what is actually set on the
extension.

This also test it in more detail by asserting the actual values that are
set, rather than if a certain method is called with what we hope is the
expected value.

Part of #252.
tombruijn added a commit that referenced this issue Jun 24, 2024
Don't assert method calls but check what the extension receives for
transaction data.

Part of #252

[skip changeset]
tombruijn added a commit that referenced this issue Jul 1, 2024
Make the assertions easier to read by describing what we want to see
happen, rather than assert some Hash of internal representation of the
transaction.

Part of #252
Part of #299

[skip changeset]
tombruijn added a commit that referenced this issue Jul 1, 2024
Make the assertions easier to read by describing what we want to see
happen, rather than assert some Hash of internal representation of the
transaction.

Part of #252
Part of #299

[skip changeset]
tombruijn added a commit that referenced this issue Jul 1, 2024
Make the assertions easier to read by describing what we want to see
happen, rather than assert some Hash of internal representation of the
transaction.

Part of #252
Part of #299

[skip changeset]
@tombruijn tombruijn self-assigned this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants