Update assets_precompile_spec.rb

[dzirtusss]

Symlink tests with tempfs. Please don't kill - I'm new to ruby/rspec/git world. Waiting for any comments.

---

This change is

[justin808]

@dzirtusss GREAT START!

@mapreal19 can you please do a quick review.

---

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

spec/react_on_rails/assets_precompile_spec.rb, line 10 [r1] (raw file):

  RSpec.describe AssetsPrecompile do

    assets_path = Pathname.new(Dir.mktmpdir)

this should either be a let block or a before block (before block requires the use of @assets_path)

I'd suggest a let block.

---

spec/react_on_rails/assets_precompile_spec.rb, line 18 [r1] (raw file):

        AssetsPrecompile.new(assets_path: assets_path).
              symlink_file(filename,digest_filename)

be sure to run rubocop -a to autofix!

you will need to run

rake lint

the red dots mean spaces.

---

spec/react_on_rails/assets_precompile_spec.rb, line 51 [r1] (raw file):

        f.close

        File.new(assets_path.join(digest_filename),"w").close

How about http://apidock.com/ruby/FileUtils/touch/class

---

spec/react_on_rails/assets_precompile_spec.rb, line 95 [r1] (raw file):

#### Problems here because in clobber dir = Rails.root.join(@generated_assets_dir)
#### which is not tmpdir, meaning we need to update real fs?

this should be a temp dir

so not sure why this won't work

---

Comments from Reviewable

[mapreal19]

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

spec/react_on_rails/assets_precompile_spec.rb, line 10 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

this should either be a let block or a before block (before block requires the use of @assets_path)

I'd suggest a let block.

let 👍

---

spec/react_on_rails/assets_precompile_spec.rb, line 12 [r1] (raw file):

    assets_path = Pathname.new(Dir.mktmpdir)

    describe "symlink_file" do

here describe "#symlink_file"

---

spec/react_on_rails/assets_precompile_spec.rb, line 24 [r1] (raw file):

      end

      it "creates a proper symlink if nested" do

better to put this inside a context "when nested"

---

spec/react_on_rails/assets_precompile_spec.rb, line 40 [r1] (raw file):

    describe "symlink_non_digested_assets" do
      it "creates the necessary symlinks" do
        manifest_filename = "manifest-alfa.json" 

create contexts for every case. It is preferable to have few expectations per each spec

---

spec/react_on_rails/assets_precompile_spec.rb, line 86 [r1] (raw file):

        a.delete_broken_symlinks

        expect(assets_path.join(filename).exist?).to be false

maybe you could use this matcher here https://www.relishapp.com/rspec/rspec-expectations/docs/built-in-matchers/exist-matcher

---

Comments from Reviewable

[justin808]

@dzirtusss Let's get the suggestions implemented and get your change merged. Thanks again!

[dzirtusss]

I have added all the changes (+rubocop) except clobber test

Last test for clobber still have problems. In assets_precompile.rb in clobber we have ABSOLUTE FIXED path as follows: dir = Rails.root.join(@generated_assets_dir) in all other places paths are RELATIVE and are set up in initialize method. So we can substitute them with any tempfs dir we want. Inside clobber it is substituted to this: Rails.root.join(tempdir) which is NOT our testing tempdir

Plus for some reason I didn't understand yet, while testing on my system Rails.root is nil and thus generates no method error (which can be my setup mistake). But, if it is right to do smth like: dir = @generated_assets_dir in clobber presuming it has full path, than everything is easy to update.

Please advise on the right logic.

[dzirtusss]

As for introducing context do U mean smth like this? Thanks

describe "symlink_non_digested_assets" do
  context "creates the necessary symlinks" do
    let (:manifest_filename) { "manifest-alfa.json" }
    let (:digest_filename) { "alfa.12345.js" }
    let (:nondigest_filename) { "alfa.js" }
    let (:digest_bad_filename) { "alfa.12345.jsx" }
    let (:nondigest_bad_filename) { "alfa.jsx" }

    before (:each) do
      Dir.chdir(assets_path)
      f = File.new(assets_path.join(manifest_filename), "w")
      f.write("{\"assets\":{\"#{nondigest_filename}\": \"#{digest_filename}\"}}")
      f.close
    end

    it "creates right symlink" do
      FileUtils.touch assets_path.join(digest_filename)

      AssetsPrecompile.new(assets_path: assets_path,
                         symlink_non_digested_assets_regex: Regexp.new('.*\.js$'))
                    .symlink_non_digested_assets

      expect(assets_path.join(digest_filename).exist?).to be true
      expect(assets_path.join(nondigest_filename).lstat.symlink?).to be true
      expect(File.identical?(assets_path.join(nondigest_filename),
                           assets_path.join(digest_filename))).to be true
    end

    it "creates right symlink with .gz" do
      FileUtils.touch assets_path.join("#{digest_filename}.gz")

      AssetsPrecompile.new(assets_path: assets_path,
                         symlink_non_digested_assets_regex: Regexp.new('.*\.js$'))
                    .symlink_non_digested_assets

      expect(assets_path.join("#{digest_filename}.gz").exist?).to be true
      expect(assets_path.join("#{nondigest_filename}.gz").lstat.symlink?).to be true
      expect(File.identical?(assets_path.join("#{nondigest_filename}.gz"),
                           assets_path.join("#{digest_filename}.gz"))).to be true
    end

    it "doesn't create wrong symlink" do
      FileUtils.touch assets_path.join(digest_bad_filename)

      AssetsPrecompile.new(assets_path: assets_path,
                         symlink_non_digested_assets_regex: Regexp.new('.*\.js$'))
                    .symlink_non_digested_assets

      expect(assets_path.join(nondigest_bad_filename).exist?).to be false
    end
  end
end

[mapreal19]

Review status: 0 of 1 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

spec/react_on_rails/assets_precompile_spec.rb, line 58 [r2] (raw file):

                        .symlink_non_digested_assets

        # testing for alfa.js symlink

@dzirtusss what I meant is having a context per each blocks of code here. Instead of having comments we could have something like:

context "correct nondigest filename" do ... end

context "zipped nondigest filename" do ... end

context "wrong nondigest filename" do ... end

Also we try to follow the four-phase test (See: https://robots.thoughtbot.com/four-phase-test) instead of abusing the use of let & before

---

Comments from Reviewable

[dzirtusss]

Review status: 0 of 1 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

spec/react_on_rails/assets_precompile_spec.rb, line 10 [r1] (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

let 👍

Done.

---

spec/react_on_rails/assets_precompile_spec.rb, line 12 [r1] (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

here describe "#symlink_file"

Done.

---

spec/react_on_rails/assets_precompile_spec.rb, line 18 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

be sure to run rubocop -a to autofix!

you will need to run

rake lint

the red dots mean spaces.

Done.

---

spec/react_on_rails/assets_precompile_spec.rb, line 24 [r1] (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

better to put this inside a context "when nested"

Done.

---

spec/react_on_rails/assets_precompile_spec.rb, line 51 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

How about http://apidock.com/ruby/FileUtils/touch/class

Done.

---

spec/react_on_rails/assets_precompile_spec.rb, line 86 [r1] (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

maybe you could use this matcher here https://www.relishapp.com/rspec/rspec-expectations/docs/built-in-matchers/exist-matcher

Done.

---

Comments from Reviewable

[dzirtusss]

Review status: 0 of 1 files reviewed at latest revision, 9 unresolved discussions.

---

spec/react_on_rails/assets_precompile_spec.rb, line 40 [r1] (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

create contexts for every case. It is preferable to have few expectations per each spec

Done.

---

spec/react_on_rails/assets_precompile_spec.rb, line 58 [r2] (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

@dzirtusss what I meant is having a context per each blocks of code here. Instead of having comments we could have something like:

context "correct nondigest filename" do ... end

context "zipped nondigest filename" do ... end

context "wrong nondigest filename" do ... end

Also we try to follow the four-phase test (See: https://robots.thoughtbot.com/four-phase-test) instead of abusing the use of let & before

Done.

---

Comments from Reviewable

[dzirtusss]

Review status: 0 of 1 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

spec/react_on_rails/assets_precompile_spec.rb, line 95 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

this should be a temp dir

so not sure why this won't work

All time getting `Rails.root == nil` in tests, seem to miss some require or initialization. Need help.

---

Comments from Reviewable

[mapreal19]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[justin808]

One small comment.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

spec/react_on_rails/assets_precompile_spec.rb, line 102 [r3] (raw file):

        expect(file).to exist
        AssetsPrecompile.new(assets_path: assets_path,
                             generated_assets_dir: assets_path).clobber

@dzirtusss

the generated_assets_dir should not be inside of the assets_path.

See this for the generated assets dir:

react_on_rails/spec/dummy/config/initializers/react_on_rails.rb

Line 14 in 1e7ab23

config.generated_assets_dir = File.join(%w(app assets webpack))

Since we're only using the generated_assets_dir here, then just don't pass the param for the assets_path.

    def clobber
      dir = Rails.root.join(@generated_assets_dir)
      if dir.present? && File.directory?(dir)
        puts "Deleting files in directory #{dir}"
        FileUtils.rm_r(Dir.glob(Rails.root.join("#{@generated_assets_dir}/*")))
      else
        puts "Could not find generated_assets_dir #{dir} defined in react_on_rails initializer: "
      end
    end

---

Comments from Reviewable

[justin808]

@dzirtusss Please see why the travis build failed per the link in "checks".

[dzirtusss]

Rails.root was nil. Didn't know how to stub )) now ok

---

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

spec/react_on_rails/assets_precompile_spec.rb, line 102 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

@dzirtusss

the generated_assets_dir should not be inside of the assets_path.

See this for the generated assets dir:

react_on_rails/spec/dummy/config/initializers/react_on_rails.rb

Line 14 in 1e7ab23

config.generated_assets_dir = File.join(%w(app assets webpack))

Since we're only using the generated_assets_dir here, then just don't pass the param for the assets_path.

    def clobber
      dir = Rails.root.join(@generated_assets_dir)
      if dir.present? && File.directory?(dir)
        puts "Deleting files in directory #{dir}"
        FileUtils.rm_r(Dir.glob(Rails.root.join("#{@generated_assets_dir}/*")))
      else
        puts "Could not find generated_assets_dir #{dir} defined in react_on_rails initializer: "
      end
    end

Done. Rails.root was nil. Didn't know how to stub )) now ok

---

Comments from Reviewable

[dzirtusss]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

spec/react_on_rails/assets_precompile_spec.rb, line 102 [r3] (raw file):

Previously, dzirtusss (Sergey) wrote…

Done. Rails.root was nil. Didn't know how to stub )) now ok

Don't passing `assets_path` generates nil error, because `assets_path.presence || default_asset_path` where it is called same `Rails` which is nil or stub

---

Comments from Reviewable

[coveralls]

Coverage increased (+4.7%) to 82.27% when pulling 3c04366 on dzirtusss:patch-1 into 1e7ab23 on shakacode:alleycat-at-git-alexey/replace_symlinks_copy.

[justin808]

Thanks @dzirtusss!

Great job!

---

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable