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

Drop Failing Spec for Circular References in Hashes #952

Merged
merged 1 commit into from Mar 2, 2020

Conversation

ksylvest
Copy link
Contributor

@ksylvest ksylvest commented Jan 30, 2020

Fix for #947.

The JSON 2.3.0 gem changed how parsing errors are raised. For example:

require 'json'
data = {}
data['cycle'] = data
JSON.dump(data)

With json v2.2.0:
This raises a SystemStackError.

With json v2.3.0:
This causes fatal (machine stack overflow in critical region)

@ksylvest
Copy link
Contributor Author

ksylvest commented Jan 30, 2020

@HazAT wanted to followup from the change introduced here: #946

I see a few different options:

  1. pin the version of json to 2.2.0 (probably not a great idea since it is a system library - also leads to lots of warnings with 2.7.0)
  2. drop the failing test here (https://github.com/getsentry/raven-ruby/blob/master/spec/raven/json_spec.rb#L64-L77)
  3. change the test to handle ruby version changes
  4. try and get this fixed in https://github.com/ruby/ruby / https://github.com/flori/json (issue: Changes in JSON.dump behaviour with cyclical structures flori/json#404)

Do you have any thoughts? #2 seems like the best option to me (since this test is a bit overbearing).

Commit that introduced spec for reference: ccb88bb

@ksylvest ksylvest marked this pull request as ready for review January 30, 2020 06:50
@ksylvest ksylvest requested a review from HazAT January 30, 2020 07:00
@ksylvest
Copy link
Contributor Author

@HazAT do you know if anyone on the sentry team is available to help with issues / PRs?

@HazAT
Copy link
Member

HazAT commented Feb 21, 2020

Hey, sorry about that.
Currently, we have no Ruby expert on the team, it's mostly me trying to keep up with everything that's going on.
So if you are interested in helping out, we are always looking for contractors, if you are interested, shoot me a mail my first name @ sentry.io :)

WRT to this PR, I would say #2 is fine.

The `JSON` 2.3.0 gem changed how parsing errors are raised. For example:

    require 'json'
    data = {}
    data['cycle'] = data
    JSON.dump(data)

With json v2.2.0:
This raises a `SystemStackError`.

With json v2.3.0:
This causes `fatal (machine stack overflow in critical region)`

Since the specs depend on 'SystemStackError' removing.
@ksylvest ksylvest changed the title Pin JSON Version to Fix Specs on Ruby HEAD Drop Failing Spec for Circular References in Hashes Mar 1, 2020
@ksylvest
Copy link
Contributor Author

ksylvest commented Mar 1, 2020

Adjusted this PR to use no. 2.

@HazAT HazAT merged commit fa3aa4a into getsentry:master Mar 2, 2020
@ksylvest ksylvest deleted the ksylvest/fix-ci-for-json branch March 2, 2020 17:18
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.

None yet

2 participants