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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eager load to make DEFAULTS deterministic #776

Merged
merged 1 commit into from
Aug 20, 2023

Conversation

michaelherold
Copy link
Contributor

This is a 馃悰 bug fix.

Summary

Configuration::DEFAULTS uses Dir.pwd to determine the root directory for the configuration. This means that lazy loading the file using autoload makes the root path for configuration non-deterministic depending on the directory location the first time you try to use Bridgetown::Configuration.

This has a knock-on effect of making tests fail nondeterministically.

By eager loading the configuration file, we can reduce this nondeterminism by ensuring the value is set the same way every time.

Context

This can be reproduced with the following on 7ecd7f6.

Reproduction
BYPASS_RESOURCE_TEST=true bundle exec ruby -Itest:lib -e 'require "./test/fixtures/test_automation.rb" ; require "./test/test_ansi.rb" ; require "./test/test_apply_command.rb" ; require "./test/test_cleaner.rb" ; require "./test/test_collections.rb" ; require "./test/test_components.rb" ; require "./test/test_configuration.rb" ; require "./test/test_configure_command.rb" ; require "./test/test_data_reader.rb" ; require "./test/test_defaults_reader.rb" ; require "./test/test_doctor_command.rb" ; require "./test/test_drop.rb" ; require "./test/test_entry_filter.rb" ; require "./test/test_erb.rb" ; require "./test/test_esbuild_command.rb" ; require "./test/test_filters.rb" ; require "./test/test_front_matter_defaults.rb" ; require "./test/test_generated_page.rb" ; require "./test/test_generated_site.rb" ; require "./test/test_kramdown.rb" ; require "./test/test_layout_reader.rb" ; require "./test/test_liquid_extensions.rb" ; require "./test/test_liquid_renderer.rb" ; require "./test/test_locales.rb" ; require "./test/test_log_adapter.rb" ; require "./test/test_model.rb" ; require "./test/test_new_command.rb" ; require "./test/test_path_sanitization.rb" ; require "./test/test_plugin_manager.rb" ; require "./test/test_relations.rb" ; require "./test/test_resource.rb" ; require "./test/test_ruby_helpers.rb" ; require "./test/test_serbea.rb" ; require "./test/test_site.rb" ; require "./test/test_site_drop.rb" ; require "./test/test_ssr.rb" ; require "./test/test_static_file.rb" ; require "./test/test_tags.rb" ; require "./test/test_utils.rb" ; require "./test/test_webpack_command.rb" ; require "./test/test_yaml_parser.rb"' -- -n "/^(?:TestEsbuildCommand#(?:test_: existing webpack project should migrate to esbuild. )|TestConfiguration#(?:test_: .from should merge input over defaults. ))$/" --seed 1234

Run this on that commit, then on this commit, and see that there's no longer a leak in the test. With this change, I think we can remove the SEED=2345 in script/test and go back to using random testing.

Configuration::DEFAULTS uses `Dir.pwd` to determine the root directory
for the configuration. This means that lazy loading the file using
`autoload` makes the root path for configuration non-deterministic
depending on the directory location the first time you try to use
`Bridgetown::Configuration`.

This has a knock-on effect of making tests fail nondeterministically.

By eager loading the configuration file, we can reduce this
nondeterminism by ensuring the value is set the same way every time.
@jaredcwhite jaredcwhite merged commit fe54a9a into bridgetownrb:main Aug 20, 2023
0 of 4 checks passed
@michaelherold michaelherold deleted the fix-test-order-bug branch August 22, 2023 02:10
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

2 participants