Skip to content

Drop Failing Spec for Circular References in Hashes#952

Merged
HazAT merged 1 commit intogetsentry:masterfrom
ksylvest:ksylvest/fix-ci-for-json
Mar 2, 2020
Merged

Drop Failing Spec for Circular References in Hashes#952
HazAT merged 1 commit intogetsentry:masterfrom
ksylvest:ksylvest/fix-ci-for-json

Conversation

@ksylvest
Copy link
Copy Markdown
Contributor

@ksylvest ksylvest commented Jan 30, 2020

Fix for https://github.com/getsentry/raven-ruby/issues/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
Copy Markdown
Contributor Author

ksylvest commented Jan 30, 2020

@HazAT wanted to followup from the change introduced here: https://github.com/getsentry/raven-ruby/pull/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 ruby/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
Copy Markdown
Contributor Author

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

@HazAT
Copy link
Copy Markdown
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
Copy Markdown
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.

2 participants