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

Upgrade ruby 3.1 #5104

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Upgrade ruby 3.1 #5104

wants to merge 4 commits into from

Conversation

DustinHaefele
Copy link
Contributor

No description provided.

@DustinHaefele DustinHaefele force-pushed the upgrade-ruby-3.1 branch 3 times, most recently from 8cad1fa to bc01a04 Compare April 23, 2024 15:03
@DustinHaefele DustinHaefele marked this pull request as ready for review April 23, 2024 15:40
@DustinHaefele DustinHaefele requested review from almalee24 and a team April 23, 2024 15:40
@almalee24
Copy link

Could you add the results for Alelo remote tests?

@DustinHaefele
Copy link
Contributor Author

@almalee24 I'm working on getting some alelo credentials to do the remote tests but I can say that the alelo unit tests fail on Ruby 3.1 with 7 OpenSSL errors if I don't update Jose to 1.2.0, but if I do update to 1.2.0 they all pass.

Ruby 3.1 and Jose 1.1.3

25 tests, 67 assertions, 0 failures, 7 errors, 0 pendings, 0 omissions, 0 notifications
72% passed

Ruby 3.1 and Jose 1.2.0

25 tests, 115 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

@@ -44,7 +44,7 @@ def test_unsuccessful_signing_request
assert gateway.options[:private_key]
assert auth = gateway.authorize(@amount, @credit_card, @options)
assert_failure auth
assert_equal 'Neither PUB key nor PRIV key: not enough data', auth.message
assert_equal 'Neither PUB key nor PRIV key: unsupported', auth.message
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new error message based on the openssl updates from the new JOSE version.

@@ -147,15 +147,15 @@ def test_failed_verify
@declined_card.number = ''
response = @gateway.verify(@declined_card, @options)
assert_failure response
assert_match %r{PAN Must be >= 13 Digits}, response.message
assert_match %r{PAN Must be >= 12 Digits}, response.message
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of the changes in this file are just updated error messaging from creditcall.

@@ -4,7 +4,7 @@ class RemoteDLocalTest < Test::Unit::TestCase
def setup
@gateway = DLocalGateway.new(fixtures(:d_local))

@amount = 200
@amount = 1000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of tests were failing with "Amount too low" so I bumped this up.

@@ -160,7 +160,7 @@ def test_successful_verify
def test_failed_verify
assert response = @gateway_auth.verify(@declined_card, @options)
assert_failure response
assert_equal 'missing: fraud_detection', response.message
assert_equal '10734: Fraud Detection Data is required', response.message
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another new error message from the gateway

@@ -60,7 +60,7 @@ def test_successful_purchase

def test_failed_purchase
assert purchase = @gateway.purchase(@success_amount, @expired_card, @options)
assert_match 'Card has expired', purchase.message
assert_match 'Transaction declined', purchase.message
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another new error message.

@DustinHaefele DustinHaefele force-pushed the upgrade-ruby-3.1 branch 2 times, most recently from 49a9918 to 24f3433 Compare April 30, 2024 18:25
@DustinHaefele
Copy link
Contributor Author

Alright here is the summary of results for the remote tests with both ruby 2.7 and ruby 3.1.

47 gateways - I could not connect to for either unexplained reasons with their sandbox or lack of credentials.

89 gateways - the remote tests had the same exact passing and failing tests in both versions of ruby.

2 gateways - had other issues detailed below

  1. Clearhaus. With the upgrade to Ruby 3.1 the error message returned from openssl was slightly different in the updated version
  2. CommerceHub - Passed 18-22% of tests, which tests passed and failed were not consistent from run to run on the same version or different versions. I have no reason to believe the upgrade affected this since it is unpredictable in both versions.

@DustinHaefele DustinHaefele requested a review from aenand May 1, 2024 17:09
@@ -2,6 +2,8 @@
= ActiveMerchant CHANGELOG

== HEAD

== Version 1.136.0 (May 1, 2024)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you release a new version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an AC on the ticket to do so. I haven't pushed the tag or done the release yet though. I was going to do that in my CAB release window.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha. You should probably cut a new version before merging this and then have this in it's own

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense to me I'll update before merging this change.

Copy link
Contributor

@aenand aenand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

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

3 participants