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

Update gems and their RBI files #1246

Merged
merged 2 commits into from
Oct 26, 2022
Merged

Update gems and their RBI files #1246

merged 2 commits into from
Oct 26, 2022

Conversation

dirceu
Copy link
Contributor

@dirceu dirceu commented Oct 25, 2022

Closes #1242
Closes #1243
Closes #1244
Closes #1245

@dirceu dirceu requested a review from a team as a code owner October 25, 2022 13:07
@vinistock
Copy link
Member

It seems that the new version of Sorbet reverts the attr_* runtime checks which is causing the test failures (we fixed them on #1227).

We'll need to revert those tests back until the attr_* runtime checks are shipped again.

@dirceu
Copy link
Contributor Author

dirceu commented Oct 26, 2022

@vinistock I reverted that and it caused failures in some of the matrix's pipelines; I removed the revert commit and other ones are failing. I'm a bit lost here as to why the it compiles signatures and structs in source files passes on Gemfile-rails-main but not on Gemfile-rails-6-1, for example. I wonder if there's some caching issue at play here. Any tips?

@vinistock
Copy link
Member

vinistock commented Oct 26, 2022

The reason is probably because we don't commit a Gemfile.lock for the extra gemfiles, so you never know for sure which version is going to be installed.

One option we can take is check the Sorbet version in the test and then change the generated RBI based on it. Something like

expected_foo_signature = if sorbet_version < 123123 && sorbet_version > 123123
  "def foo=(arg0); end"
else
  <<~RUBY
    sig { ... }
    def foo=(foo); end
  RUBY
end

st0012 added a commit that referenced this pull request Oct 26, 2022
1. Latest Sorbet reverted a runtime check, so we should revert the "foo" signature
   we changed for that version too.
   More info: #1246 (comment)
2. Because the extra gemfiles don't lock Sorbet version, it always uses the latest one.
   But the tests running with Gemfile runs against the old Sorbet. So we get inconsistant
   CI result on the same test.
3. To make all tests pass, we need to do the 2 things at the same time:
    1. Upgrade Sorbet
    2. Revert the signature
st0012 added a commit that referenced this pull request Oct 26, 2022
1. Latest Sorbet reverted a runtime check, so we should revert the "foo" signature
   we changed for that version too.
   More info: #1246 (comment)
2. Because the extra gemfiles don't lock Sorbet version, it always uses the latest one.
   But the tests running with Gemfile runs against the old Sorbet. So we get inconsistant
   CI result on the same test.
3. To make all tests pass, we need to do the 2 things at the same time:
    1. Upgrade Sorbet
    2. Revert the signature
@st0012
Copy link
Member

st0012 commented Oct 26, 2022

I solved the issue in #1247 so this PR should be passing after rebasing/merging main.

@st0012 st0012 added the dependencies Pull requests that update a dependency file label Oct 26, 2022
@dirceu dirceu merged commit 98fbbee into main Oct 26, 2022
@dirceu dirceu deleted the update-gems-20221025 branch October 26, 2022 19:44
@shopify-shipit shopify-shipit bot temporarily deployed to production November 10, 2022 17:34 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants