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

Doc updates, fix test helper #1093

Closed
wants to merge 6 commits into from

Conversation

justin808
Copy link
Member

@justin808 justin808 commented May 23, 2018

Test helper for detecting stale bundles did not properly handle the case
of a server-bundle.js without a hash.

This is reproducible:

  1. git clone https://github.com/shakacode/react-webpack-rails-tutorial
  2. bundle && yarn
  3. rspec

See that the server-bundle.js is reported as missing.


This change is Reviewable

@justin808 justin808 force-pushed the fix-test-helper-for-server-bundle branch from ce6ec06 to 87ff7e8 Compare May 23, 2018 09:06
@justin808 justin808 force-pushed the fix-test-helper-for-server-bundle branch 2 times, most recently from 57e81c7 to 571dd45 Compare June 6, 2018 01:30
@mapreal19
Copy link
Member

:lgtm: Few minor suggestions


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


lib/react_on_rails/prerender_error.rb, line 6 at r1 (raw file):

module ReactOnRails
  class PrerenderError < ::ReactOnRails::Error
    # TODO: Consider remove providing original `err` as already have access to `self.cause`

do we create an issue so we don't forget and maybe a newcomer could take this?


lib/react_on_rails/version_syntax_converter.rb, line 5 at r1 (raw file):

require_relative "version"

module ReactOnRails

👍


spec/react_on_rails/configuration_spec.rb, line 11 at r1 (raw file):

    let(:using_webpacker) { false }

    after do

I'd put the after block after the before block


spec/react_on_rails/utils_spec.rb, line 87 at r1 (raw file):

        end

        it { expect(subject).to end_with("public/webpack/development/webpack-bundle.js") }

nice, wasn't aware of end_with


spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb, line 60 at r1 (raw file):

      context "when using webpacker, a missing server bundle without hash, and manifest is current" do
        let(:webpack_generated_files) { %w[manifest.json server-bundle.js] }

not for this PR but for tests in the future I'd try to stay of using let for setup.

Is a cop in the rubocop-rspec: https://github.com/rubocop-hq/rubocop-rspec/blob/master/lib/rubocop/cop/rspec/let_setup.rb#L31


Comments from Reviewable

@mapreal19 mapreal19 self-assigned this Jun 6, 2018
This is the case that is fixed:

Test helper for detecting stale bundles did not properly handle the case
of a server-bundle.js without a hash.

This is reproducible:

git clone https://github.com/shakacode/react-webpack-rails-tutorial
bundle && yarn
rspec
See that the server-bundle.js is reported as missing.
@justin808 justin808 force-pushed the fix-test-helper-for-server-bundle branch from 2b07cc1 to a7f79de Compare June 14, 2018 01:02
@coveralls
Copy link

coveralls commented Jun 15, 2018

Coverage Status

Coverage remained the same at ?% when pulling d85d8c5 on fix-test-helper-for-server-bundle into aaef7c0 on master.

@justin808
Copy link
Member Author

broke into #1102 and #1101

@justin808 justin808 closed this Jun 15, 2018
@justin808 justin808 mentioned this pull request Jun 15, 2018
@justin808 justin808 deleted the fix-test-helper-for-server-bundle branch June 17, 2018 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants