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

Move MRI build to Github Actions #2404

Merged
merged 9 commits into from Nov 23, 2020
Merged

Move MRI build to Github Actions #2404

merged 9 commits into from Nov 23, 2020

Conversation

benoittgt
Copy link
Member

@benoittgt benoittgt commented Nov 18, 2020

JRuby not moved yet because there is too many issues with it at the moment.

To do in another PR?

  • fix ruby 3
  • fix flaky test with "mountable_engine?" error
  • add JRuby

@benoittgt benoittgt force-pushed the move-to-github-actions branch 3 times, most recently from e6bfef7 to e64e5af Compare November 18, 2020 23:18
jruby not moved because there is too many issues with it at the moment.

Next step fix :
- fix ruby 3
- fix flacky test with "mountable_engine?" error
- add JRuby
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Wonderful! Thanks!

PS mountable_engine? strikes back. Unrelated, it was the case for long.

@benoittgt
Copy link
Member Author

Ok. For the flaky test. I nearly find it.

If you have this

# spec/generators/rspec/scaffold/scaffold_generator_spec.rb
# Generators are not automatically loaded by Rails
require 'generators/rspec/scaffold/scaffold_generator'
require 'support/generators'

RSpec.describe Rspec::Generators::ScaffoldGenerator, type: :generator do
  setup_default_destination

  describe 'standard request specs' do
    subject { file('spec/requests/posts_spec.rb') }

    describe 'with --no-request_specs' do
      before { run_generator %w[posts --no-request_specs] }
      it { is_expected.not_to exist }
    end

    describe 'in an engine' do
      before do
        allow_any_instance_of(::Rails::Generators::NamedBase).to receive(:mountable_engine?).and_return(true)
        run_generator %w[posts --request_specs]
      end

      it do
        is_expected.to contain('Engine.routes.url_helpers')
      end
    end
  end
end

bundle exec rspec spec/generators/rspec/scaffold/scaffold_generator_spec.rb --seed 58348, the stub have to be run first.

It seems the stub is leaking to other test. We are not able to reset properly to the right state after the engine test pass.

It's very late here. So maybe tomorrow I will try to keep digging, but if you have an idea... Maybe it is related to ammeter?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
allow_any_instance_of(::Rails::Generators::NamedBase).to receive(:mountable_engine?).and_return(true)
run_generator %w[posts --request_specs]
it 'generates files with Engine url_helpers' do
in_sub_process do
Copy link
Member

Choose a reason for hiding this comment

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

👏

Wondering why it was wiping out the original method along the way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I suspect, and this is just a theory on my part, because when it stubs the method doesn't exist, it's then loaded, and the then stub blows it away as it was never there originally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

🤯 😱
Reminds me of #2394 (comment)

Error was

+  1) InboxMailbox route email to properly mailbox
+     Failure/Error:
+       expect(InboxMailbox)
+         .to receive_inbound_email(to: "replies@example.com")
+
+     ArgumentError:
+       wrong number of arguments (given 1, expected 0)
+     # /home/runner/work/rspec-rails/bundle/ruby/3.0.0/gems/actionmailbox-6.0.3.4/lib/action_mailbox/test_helper.rb:16:in `create_inbound_email_from_mail'
+     # ./spec/mailboxes/inbox_mailbox_spec.rb:6:in `block (2 levels) in <top (required)>'

It should be fixed with Rails 6.1 rails/rails#35904
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Co-authored-by: Jon Rowe <hello@jonrowe.co.uk>
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@@ -89,5 +89,6 @@ jobs:
with:
ruby-version: ${{ matrix.ruby }}
- run: script/clone_all_rspec_repos
- run: bundle
Copy link
Member

Choose a reason for hiding this comment

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

I had to restore the script to update bundler to get the main builds working 🤔 😬

env:
RAILS_VERSION: '~> 5.0.0'
env: ${{ matrix.env }}
continue-on-error: ${{ matrix.allow_failure || false }}
Copy link
Member Author

@benoittgt benoittgt Nov 22, 2020

Choose a reason for hiding this comment

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

I doesn't seem to work. :/
https://github.com/rspec/rspec-rails/runs/1438441078?check_suite_focus=true

I am in favor of moving it to script/run_build step.

Copy link
Member

Choose a reason for hiding this comment

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

SyntaxError: /home/runner/work/rspec-rails/bundle/ruby/2.2.0/gems/actionpack-5.2.4.4/lib/action_dispatch/request/session.rb:96: syntax error, unexpected '.'
          id&.public_id

That build is failing genuinely.

Copy link
Member

Choose a reason for hiding this comment

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

I think they introduced a 2.2.x bug for 5.2... &. is 2.3 and above.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, I've missed 2.2.2 is tested against 5-2-stable already.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think they introduced a 2.2.x bug for 5.2... &. is 2.3 and above.

Yes completely and I didn't see a patch to change that ATM.

@benoittgt
Copy link
Member Author

I think the PR can be merged. I will have extra work for 6.1.0rc1 that I will probably do in #2400

I will try to move JRuby over GA in another PR.

@JonRowe JonRowe merged commit 9217638 into main Nov 23, 2020
@JonRowe JonRowe deleted the move-to-github-actions branch November 23, 2020 09:30
@pirj
Copy link
Member

pirj commented Nov 23, 2020

🎉

@benoittgt
Copy link
Member Author

Yeehaa

JonRowe added a commit that referenced this pull request Nov 26, 2020
JonRowe added a commit that referenced this pull request Dec 26, 2020
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