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

Clarification and reorganization of test conditions in configuration specs #1607

Open
Alejandroq12 opened this issue Apr 6, 2024 · 1 comment

Comments

@Alejandroq12
Copy link

While reviewing and checking on the Configuration specs within the ReactOnRails module, I've identified an area where we can improve the readability and organization of the test conditions, particularly concerning the use of the let(:using_webpacker) setup.
https://github.com/shakacode/react_on_rails/blob/master/spec/react_on_rails/configuration_spec.rb#L1-L305

Is your feature request related to a problem? Please describe.
The current tests utilize let(:using_webpacker) { false } at the beginning and then override this condition within specific tests to true when needed. This approach, while flexible, can lead to confusion as the override happens in the middle of the test file, making it harder to track which condition applies to which set of tests.

Describe the solution you'd like
I propose that we reorganize these tests using RSpec's context blocks to group related tests under clear conditions. Each context will have its let and before blocks, making it immediately apparent which setup applies to which group of tests. This structure not only improves readability but also aligns with best practices by clearly separating different testing scenarios.

Describe alternatives you've considered
1- Use of Contexts: Define two main contexts: context "when not using webpacker" and context "when using webpacker". This distinction will group tests based on whether webpacker is being used, making the test conditions clearer.

2- Specific Setup per Context: Move the let(:using_webpacker) and related before blocks into their respective contexts. This ensures that each test group has its setup, avoiding the need for mid-file overrides.

Additional context
Benefits:

  • Clarity: It will be easier for developers to understand at a glance which conditions apply to which tests.
  • Maintainability: New tests can be added to the appropriate context without worrying about affecting unrelated tests.
  • Best Practices: Adheres to RSpec's intended use of context for organizing tests under specific conditions.

Example Refactoring:

module ReactOnRails
  RSpec.describe Configuration do
    let(:existing_path) { Pathname.new(Dir.mktmpdir) }
    let(:not_existing_path) { "/path/to/#{SecureRandom.hex(4)}" }

    before do
      ReactOnRails.instance_variable_set(:@configuration, nil)
    end

    after do
      ReactOnRails.instance_variable_set(:@configuration, nil)
    end

    # Use contexts to separate tests with different conditions
    context "when not using webpacker" do
      let(:using_webpacker) { false }

      before do
        allow(ReactOnRails::WebpackerUtils).to receive(:using_webpacker?).and_return(using_webpacker)
      end

      # Place tests that operate under the condition `using_webpacker` is false
    end

    context "when using webpacker" do
      let(:using_webpacker) { true }
      let(:webpacker_public_output_path) { File.expand_path(File.join(Rails.root, "public/webpack/dev")) }

      before do
        allow(ReactOnRails::WebpackerUtils).to receive(:using_webpacker?).and_return(using_webpacker)
        allow(Rails).to receive(:root).and_return(File.expand_path("."))
        allow(Webpacker).to receive_message_chain("config.public_output_path").and_return(webpacker_public_output_path)
      end

      describe "generated_assets_dir" do
        it "does not throw if the generated assets dir is blank with webpacker" do
          expect do
            ReactOnRails.configure do |config|
              config.generated_assets_dir = ""
            end
          end.not_to raise_error
        end

        it "does not throw if the webpacker_public_output_path does match the generated assets dir" do
          expect do
            ReactOnRails.configure do |config|
              config.generated_assets_dir = "public/webpack/dev"
            end
          end.not_to raise_error
        end

        it "does throw if the webpacker_public_output_path does not match the generated assets dir" do
          expect do
            ReactOnRails.configure do |config|
              config.generated_assets_dir = "public/webpack/other"
            end
          end.to raise_error(ReactOnRails::Error, /does not match the value for public_output_path/)
        end
      end

      # Add additional "when using webpacker" tests here
    end

    # Add any additional contexts or tests here

  end
end
@justin808
Copy link
Member

Please submit it as a PR and I'll check the diffs.

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

No branches or pull requests

2 participants