-
Notifications
You must be signed in to change notification settings - Fork 111
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
Enable Rubocop Rule to Freeze Constants #10340
Conversation
2ba0e63
to
37762a0
Compare
@unused_keys = config_map.keys - config.written_env.keys | ||
@key_types = config.key_types.freeze | ||
@unused_keys = (config_map.keys - config.written_env.keys).freeze | ||
config.written_env.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have taken advantage of testing things in the Rails conosle by modifing IdentityConfig.store.foo = :bar
, I'm sure I could work around it but I would skip this if given the option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, in development? We could only enable in test/prod as an alternative too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😬 no in staging, to test API keys, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In testing this, that functionality will still work since it depends on the struct itself being frozen, which this doesn't do.
37762a0
to
3d345ad
Compare
dc093b2
to
983ca3c
Compare
app/presenters/idv/in_person/verification_results_email_presenter.rb
Outdated
Show resolved
Hide resolved
d4af174
to
3209637
Compare
d781305
to
8ccdcb9
Compare
1f51f6a
to
0713b8d
Compare
b091ec7
to
c964099
Compare
RSpec.describe AbTests do | ||
def reload_ab_test_initializer! | ||
# undefine the AB tests instances so we can re-initialize them with different config values | ||
AbTests.constants.each do |const_name| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are difficult to refactor as frozen constants, though it is in theory possible. The tests are mostly tests covering the behavior already tested in spec/lib/ab_test_bucket_spec.rb
.
|
||
expect(result).to eq(doc_auth_vendor) | ||
end | ||
end | ||
|
||
context 'with a discriminator that hashes inside the test group' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to https://github.com/18F/identity-idp/pull/10340/files#r1550503287, this test group is mostly testing the bucket class rather than the implementation of it in DocAuthRouter
. The test above for the nil discriminator is unique to the DocAuthRouter and won't be removed.
changelog: Internal, Performance, Freeze constants Co-authored-by: Zach Margolis <zachary.margolis@gsa.gov>
…ter.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
c964099
to
69f5bc6
Compare
🛠 Summary of changes
Testing the waters on this a bit. Ruby constants warn when re-assigned, but are still mutable. Short illustration of the potential issue:
This PR enables the Style/MutableConstant Rubocop rule in it's default configuration. We could experimentally enable it in
strict
mode to try to freeze non-literal constants.